-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Remove useless assignments #11774
Remove useless assignments #11774
Conversation
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.
LGTM, just a minor suggestion.
src/mime/media_type.cr
Outdated
encoding, _split_one, rest = encoded.partition('\'') | ||
_lang, _split_two, value = rest.partition('\'') |
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.
I think we can just use underscore without names to make the relevant variable names pop out more.
encoding, _split_one, rest = encoded.partition('\'') | |
_lang, _split_two, value = rest.partition('\'') | |
encoding, _, rest = encoded.partition('\'') | |
_, _, value = rest.partition('\'') |
Maybe keep _lang
as a label, but I'd certainly toss out the separator names.
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.
_lang
would still be unused. Does Ameba intentionally skip over variables starting with an underscore? (I know that Elixir does, for example.)
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.
Yes, I think the purpose of variable names with underscore prefix is to indicate that variable is unused, but still give it a label to document context.
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.
I fixed split
word, and use the lang with the _
prefix, linter doesn't worry about this line now
Hey! this is my first
crystal
contribution. I ran the source code through the ameba analyzer (ttps://github.com/crystal-ameba/ameba) and fixed a few lines.If it's helpful I can correct others comments.
The pull request is in the draft stage, if anything needs to be fixed, I'd love some feedback.