google
[*.py] Lint
I'm a little worried that this PR changes too much and thus will never be merged.
The first commit is from:
fd -HIepy -x bash -c 'autoflake --remove-all-unused-imports -i "$0" && isort --atomic "$0" && python -m black "$0"' {} \;
The goal is to have all these lint checks passing:
flake8 . --count --select=$(printf '%s,' {A..Z}) --ignore='W503,E203,E402,E731,E741,C901' --show-source --max-line-length=119 --statistics
pylint flax -rn --load-plugins=pylint.extensions.check_docs --accept-no-param-doc=n
This is related to this PR #933
We would have to review this carefully as it would break the git blame chain, and that would happen every time we change lint configuration going forward
@avital Yep, I agree completely. So that's why it's important to get linters setup as early as possible in the project. Introducing these to TensorFlow for example would be completely untenable [without some executive decision: good luck], as there are over 200 open PRs.
In terms of what linters pass currently, all the flake8 tests pass. Just wanted to wait for your feedback before proceeding with updating [almost] all the docstrings.
We should probably update the pylint configuration with the style that is used right now. We need at least 2 space indent instead of 4 space otherwise pretty much every line changes
@jheek One can just add this line to the GitHub Action:
flake8 . --count --select=$(printf '%s,' {A..Z}) --ignore='W503,E203,E402,E731,E741,C901' --show-source --max-line-length=119 --statistics
In terms of two spaces versus four, I suppose I can modify that easily, but not sure that's good practice. With the exception of Google repositories, most every repo I see is four spaces (not tabs). Anyway I'm happy if a little surprised that you're actually considering merging this that changes so many lines.
Good!
I think it's best to pick the dominant style of coding as source of truth for the linting rules for now. That way the issue doesn't become too invasive. We can always switch some style in the codebase to a different indentation or string quote but that shouldn't be in the linting PR. Otherwise we are going to mix a discussion about consistency (which we obviously want) with subjective discussions like indentations (which obviously everyone is going to disagree on)
Closing this due to inactivity. Also, we are incrementally fixing linting errors in our codebase, but this is quite a difficult process due to how we set up our linting infrastructure, so submitting it all in one PR is unfortunately not feasible.
Hi @SamuelMarks, I was going over all PRs and issues and realize now I probably have closed this PR too soon.
Apologies! I didn't mean to disregard all your hard work. We are trying to minimize the number of open issues so we can keep maintaining our repo effectively.
I have now read both this PR and the discussion in #841 in more detail.
So that's why it's important to get linters setup as early as possible in the project. Introducing these to TensorFlow for example would be completely untenable [without some executive decision: good luck], as there are over 200 open PRs.
I share this sentiment! One problem is that we are internally using pylint (which is a google product), but the external version is slightly different, so if we decide to enforce it we will run into problems.
The pylint team recommended using something like this: https://github.com/google/styleguide/blob/gh-pages/pylintrc
Anyway I am reopening this issue, sorry again!