__all__ to module-level #1441
__all__ to module-level #1441
Conversation
|
@janiversen the pylint error being encountered here has nothing to do with this PR. I think this is good to go 👍 |
|
Well dev did not report that error, so you have changed something. |
.pre-commit-config.yaml
Outdated
| - id: check-added-large-files | ||
| - repo: https://github.com/pycqa/isort | ||
| - id: trailing-whitespace | ||
| - id: end-of-file-fixer |
There was a problem hiding this comment.
Please do not activate that, we do not want it.
.pre-commit-config.yaml
Outdated
| rev: 5.12.0 | ||
| hooks: | ||
| - id: isort | ||
| - id: isort |
There was a problem hiding this comment.
@alexrudd2 all these “- “ to “ -“ changes are you ok with them ? seems like not needed to me.
There was a problem hiding this comment.
to me it looks as a change just to make a change, but it is your turf.
There was a problem hiding this comment.
I run yamllint locally and it was complaining
These errors are all fixed now.
There was a problem hiding this comment.
Well our CI did not complain, so you are running something we do not.
There was a problem hiding this comment.
our precommit runs a yaml test, and do not complain. If you run a linter locally place make sure it runs with the same options.
There was a problem hiding this comment.
Okay sure, reverted most of the changes just now
There was a problem hiding this comment.
Okay per your request jan I reverted this entire file, even though I believe the changes were valuable
There was a problem hiding this comment.
I tried to explain to you do not mix issues in the same pull request, that is the important point. Whether or not they are valuable is something to be discussed outside this PR.
feel free to submit another Pull request with the end_of_file_fixer, then we can discuss that change, which can include a reformatting of pre_commit.
There was a problem hiding this comment.
Yep I hear that, I made #1442
One other comment is in CI, something like pre-commit run --from-ref origin/dev --to-ref HEAD can be interesting.
It means pre-commit is scoped to that particular PR, not run on files outside of it. This technique is sometimes called "incremental". One pro of this is one can make pre-commit config tweaks, without having to make repo wide changes (currently mandated by --all-files). One con of this is it can be seen as adding tech debt, since the remainder of the repo is sort of "stale" with respect to the pre-commit config. I am not contributing this, more just an alternate balance I have seen struck.
There was a problem hiding this comment.
No in CI we try to run on the whole source tree, because we have all to often seen side effects in files not touched.
Locally it needs to be fast so it runs only on the changed files and not with all CI checks.
| THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
| (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF | ||
| THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
|
|
| include requirements.txt | ||
| include README.rst | ||
| include CHANGELOG.rst | ||
| include LICENSE |
There was a problem hiding this comment.
There was a missing empty line here, hence GitHub with the red symbol. This standardizes all files to have one empty line at the end
There was a problem hiding this comment.
My point was, please do not touch the file endings, all files are not equal.
There was a problem hiding this comment.
Why aren't they equal?
Imo this is a one-time fix, just standardize every line to have one newline:
- Better for GitHub (no red warning symbols)
- Works with everyone's IDE
- One line plays best when scrolling to EOF
- Having
end-of-file-fixerstandardizes all files, which imo is minor win
At multiple companies in multiple industries we roll with this, and it's fine imo.
There was a problem hiding this comment.
Multiple companies have multiple policies and standards....we are lucky we only have our own :-)
I will review your PR again, when CI is green, but you have to find better arguments that "multiple....." to convince me that adding end-of-file-fixer is worth it.
There was a problem hiding this comment.
Haha yeah sure. CI is green, if you would like me to remove end-of-file-fixer, lemme know
There was a problem hiding this comment.
I do not want that check, at least not in this PR. If we want that check (and it is a big if) it should be done "loudly" in its own PR.
| ) | ||
|
|
||
|
|
||
| # ---------------------------------------------------------------------------# |
There was a problem hiding this comment.
@janiversen I removed this to fix the pylint message, I believe this was a stale comment anyways
|
@jamesbraza your original issue with all would have been merged fast, had you not blended in unrelated changes. Changing CI/precommit etc are a lot more sensitive, because it affects our daily work. |
Part of it is I was using |
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
you still have a number of changes not related to "all", please clean that.
We can discuss the other changes in another PR.
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
mostly small leftovers.
| Patch for serial_asyncio, also submitted to the repo: | ||
|
|
||
| https://github.com/pyserial/pyserial-asyncio/pull/94 | ||
|
|
There was a problem hiding this comment.
We do NOT change in a copied module. Apart from that I believe it has nothing to do with the PR.
|
Okay, I leave this PR in your hands now, I think it's good to go! Feel free to make changes to the branch directly |
3 participants
Motivation: #1437 (comment)
While I was at it, I cleaned up the pre-commit config and added the
end-of-file-fixerhook for clean GitHub diff.