-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
lll: skip imports #3288
lll: skip imports #3288
Conversation
Hey, thank you for opening your first Pull Request ! |
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 for the PR!
I added some suggestions about simplifying the code and also a question about correctness. Maybe worth adding a unit test for this?
Also I'm curious if we should set lineNumber := 0
and do lineNumber++
on line 96. That way you don't need to do that in every new branch you create and just call continue
since we should always increment the number on each line we scan.
Thank you for your reply! I will try to fix all comments. One question: could you please guide me where should I put unit test for lll? I don't see any *_test file in the golinters directory. Should I put it there? |
@ibez92 you have to sign the CLA |
d3309e7
to
9ad980b
Compare
Yeah, missed my email in commit, sry. Now it's signed |
Have a look at testdata. You can probably just set max lengt in the config to something very short and import any internal package. |
Thank you! I added 2 tests for single and multi import |
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 but can we also add the change to lineNumber
to avoid the incrementation at three different places? Basically:
- Set
lineNumber := 0
on line 87 - Do
lineNumber++
on line 92 - Remove
lineNumber++
in all branches (line 97, 106 and 121)
That way we can add any logic and rely on line numbers getting incremented even if we do a continue
in a branch.
Nice point, thank you! Did as you suggested |
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, not merging until you've updated your review status @ldez to avoid miscommunication.
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
Skip long line check on imports.
Fixes #3251