Linting updates#12773
Conversation
…s because I wasn't sure what to do there.
| use-flake8-tabs = True | ||
| # Not all checks are replaced by flake8-tabs, however, pycodestyle is still not compatible with tabs. | ||
| use-pycodestyle-indent = False | ||
| continuation-style = hanging |
There was a problem hiding this comment.
Why you've removed this line?
There was a problem hiding this comment.
This one is still unanswered.
| #This file is covered by the GNU General Public License. | ||
| #See the file COPYING for more details. | ||
| #Copyright (C) 2007-2010 Michael Curran <mick@kulgan.net>, James Teh <jamie@jantrid.net> | ||
| # vkCodes.py |
There was a problem hiding this comment.
This line, and all lines which just contains the file name should be removed according to the copyright header standards on the Wiki. Also it feels a bit weird to me to modify the file just to reformat its copyright header so this should probably be reverted.
|
Ah, my bad. Fixing both now. :) |
|
Hi, this should all be fixed now. :) |
|
Aaaah, my bad. Should be fixed now. |
|
Oh, I missed that. The reason I removed the line in flake8.ini was because it produced errors on every line, even comments, and I have no idea why. |
|
What version of flake8-tabs are you running? Current requirements file states that version 2.1.0 should be used. |
|
2.3.2. Would that effect it? |
|
Yup, that fixed it. Sorry. Re-added it. |
|
Hi, I think a better approach to linting is looking for specific Flake8 errors first (say, E501), test changes, commit the result, then move onto the next error. You can also do this at the package level where you go through a package (say, control types), correct Flake8 errors, do individual commits for each error, then move onto the next package. I advise not linting the entire source code yet – keep in mind that there are outstanding pull requests that might as well receive the lint treatment at some point (this is why I perform lint locally before committing changes). Thanks.
|
| os.path.basename(ref), | ||
| commit[:7]) | ||
| except: | ||
| except: # noqa: E722 |
There was a problem hiding this comment.
I believe this could be fixed by the following:
| except: # noqa: E722 | |
| except Exception: |
There was a problem hiding this comment.
Or even better spend a little time understanding this function and catch only exceptions which can be raised here.
|
|
||
| import os | ||
| from buildVersion import * | ||
| from buildVersion import * # noqa: F403, F401 |
There was a problem hiding this comment.
This isn't really a fix. Instead, import the required values for the strings directly, and change .format calls to use these variables.
There was a problem hiding this comment.
A lot of code - both in NVDA and in add-ons relies on versionInfo star-importing everything from buildVersion. If we really want to go this route then this would need to wait until 2022.1.
| self._commandList.append(textInfos.FieldCommand("formatChange", newAttrs)) | ||
| else: | ||
| raise ValueError("Unknown tag name: %s"%tagName) | ||
| raise ValueError("Unknown tag name: %s" % tagName) |
There was a problem hiding this comment.
If we are changing this line, we should use the new standard of f-strings
| raise ValueError("Unknown tag name: %s" % tagName) | |
| raise ValueError(f"Unknown tag name: {tagName}") |
| pass | ||
| else: | ||
| raise ValueError("unknown tag name: %s"%tagName) | ||
| raise ValueError("unknown tag name: %s" % tagName) |
There was a problem hiding this comment.
| raise ValueError("unknown tag name: %s" % tagName) | |
| raise ValueError(f"unknown tag name: {tagName}") |
|
I don't think we can accept purely linting changes like this. Our current approach is to fix linting errors as we modify code. This preserves the 'git blame' history. I'm sorry to say this after you have spent time on this. To prevent this in the future, please create an issue and make sure it gets attention (follow up on the developers mailing list etc). If we were to do a mass "lint fix only" change, it will need the following properties:
As a first step we plan to look at normalizing the line endings across the code base. Projects such as Black/Tan are code formatters for Python. They or something similar could be used to automate the re-format effort. I'd like to close this without merging. |
Link to issue number:
None
Summary of the issue:
I know we've been trying to cut back on linting issues. I tried to tackle this a few months back, but didn't really know what I was doing. Let me know how it looks now.
Description of how this pull request fixes the issue:
I linted a few files (buildVersion.py, XMLFormatting.py, wincon.py, and a few others). I've also created a new branch (lintingUpdates), if people wish to use it for future changes such as these.
Testing strategy:
NVDA runs from source with no issues.
Known issues with pull request:
None
Change log entries:
B=None
Code Review Checklist: