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
[Docker][Pylint] Use regexes for good names #11712
Conversation
Will need to upgrade the CI images before merging this BTW. |
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.
Furthermore we partially relax rules for variable names to allow most 1-2 lower case character names and 1 character upper case names (which are used a lot in the tests).
Can we separate this out from this PR? The current tests are often hard to read because of this poor variable naming and that we should have a separate discussion as to whether we want to do this.
Certainly: #11734. Now re: discussion on changing variable names. Right now we allow the following whitelist of 'bad' variable names:
I think the new regexes are simply an extension of the status quo (as if we allow 'a' and 'b' as variable names we should also allow 'd') but in a more readable format. |
@AndrewZhaoLuo I suggest we head in the direction of descriptive variable names and limit the single letter variables as much as we can, if we look at the C++ style guide it has a section detailing best practices for naming: For tests this is extra important, as the single letter variables make it harder to follow the logic of the test. For example, why is |
I agree with you in principle, however as it stands we do allow 'bad' variable names, however they are limited to a very small list which is fairly arbitrary. It seems to me it was basically made in order for the linter to pass. We can always restrict things later, but as it stands right now it provides an inconsistent policy which makes #11414 a bit harder to do. |
Is there any ideas to either merge this patch, or modify this to remove all the single variable names altogether. When modifying the pytests to be linter compliant, I normally just run the linter and fix the errors it reports (I guess others do it in a similar fashion). As it stands now, the linter errors for variable |
@quic-sanirudh I haven't found time to get to this. I think your plan of closing the loophole is a good one. However, it is another major refactor |
Now that we are cleaning up the test directory (see #11414), I thought this might be a good time to update pylint.
Depends on #11733
Furthermore we partially relax rules for variable names to allow most 1-2 lower case character names and 1 character upper case names (which are used a lot in the tests).
Motivation:
A lot of tests are like:
Where capital letters represent TE placeholder tensors which seems reasonable to me.