-
Notifications
You must be signed in to change notification settings - Fork 121
feat: remove skip_clippy_and_linting
flag from build
command + add lint
command
#2015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: remove skip_clippy_and_linting
flag from build
command + add lint
command
#2015
Conversation
skip_clippy_and_linting
flag from build
command + add lint
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick review at it and it looks good :)
Have left a couple of comments. One regarding the change of the name for the option --lint
within the lint
command so it reads less repetitive and the name a bit more meaningful given this new context.
The other comment is a nit about a potential refactor of module linting
which I am not sure might be worth the effort, haven't either look for usages of that module across the codebase.
/// | ||
/// Basic clippy lints are deemed important and run anyway. | ||
#[clap(long)] | ||
lint: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is only used to run extra lints via dylint
aside from clippy
lints. Might be worth changing the name of the option to extra-lints
or dylint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I have renamed it to dylint
and updated the comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chungquantin @al3mart dylint
is an implementation detail, so extra-lints
is a better user-facing name if we're renaming this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidsemakula I see, that makes sense and more user friendly. I reverted back to extra-lints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
One consideration would be adding in some deprecation messages for the options we are removing. But I think we should be mostly OK if we decide not doing so.
This reverts commit d9fd093.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Except for one high-level comment 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chungquantin
* feat: if not mapped yet * chore: fix method name and add CHANGELOG * fix: clippy warnings * original_account_entry * chore: minor fixes * chore: add new error type * chore: Remove `skip_clippy_and_linting` from contract_build args See use-ink/cargo-contract#2015 --------- Co-authored-by: davidsemakula <[email protected]>
Summary
Closes #1989
ink
orpallet-contracts
?Description
build/lib.rs
to a separatelint.rs
file.skip_clippy_and_linting
flag from thecargo contract build
command. Adjust thelint
flag to requires the--generate=check-only
if the flag is provided.lint_code_only
test.Checklist before requesting a review
CHANGELOG.md