Skip to content
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

Update black integration to say what is wrong and fix python versions #362

Merged

Conversation

freemansw1
Copy link
Member

Coming from a frustrating experience developing #354, update our black integration to specify what it would change and also set the versions to ensure compatibility. We will need to update this file when we bump versions (on either end).

  • Have you followed our guidelines in CONTRIBUTING.md?
  • Have you self-reviewed your code and corrected any misspellings?
  • Have you written documentation that is easy to understand?
  • Have you written descriptive commit messages?
  • Have you added NumPy docstrings for newly added functions?
  • Have you formatted your code using black?
  • If you have introduced a new functionality, have you added adequate unit tests?
  • Have all tests passed in your local clone?
  • If you have introduced a new functionality, have you added an example notebook?
  • Have you kept your pull request small and limited so that it is easy to review?
  • Have the newest changes from this branch been merged?

@freemansw1 freemansw1 changed the base branch from main to RC_v1.5.x November 8, 2023 16:59
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (85f8f3a) 56.35% compared to head (1d510e1) 56.68%.
Report is 37 commits behind head on RC_v1.5.x.

Additional details and impacted files
@@              Coverage Diff              @@
##           RC_v1.5.x     #362      +/-   ##
=============================================
+ Coverage      56.35%   56.68%   +0.33%     
=============================================
  Files             16       19       +3     
  Lines           3384     3426      +42     
=============================================
+ Hits            1907     1942      +35     
- Misses          1477     1484       +7     
Flag Coverage Δ
unittests 56.68% <ø> (+0.33%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
tobac/analysis.py 8.48% <ø> (ø)
tobac/plotting.py 3.05% <ø> (ø)

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@freemansw1
Copy link
Member Author

Interesting that when we add specific versions, black comes up with some new things for us to fix. I'm happy to remove the specific versions (as long as we leave the diff). @JuliaKukulies You did the initial integration here; what do you think?

@w-k-jones
Copy link
Member

Interesting, I'm not sure why previous formatting jobs haven't caught these issues

@freemansw1
Copy link
Member Author

Interesting, I'm not sure why previous formatting jobs haven't caught these issues

Yes when you run just black tobac, it doesn't have anything, but black tobac -t py37 -t py38 -t py39 -t py310 -t py311 comes up with all sorts of stuff. Not a clue why. If we switched, we would need to update our pre-commit too I've just realized, and also everyone would need to update their IDEs (ugh).

@JuliaKukulies
Copy link
Member

Interesting, I'm not sure why previous formatting jobs haven't caught these issues

Yes when you run just black tobac, it doesn't have anything, but black tobac -t py37 -t py38 -t py39 -t py310 -t py311 comes up with all sorts of stuff. Not a clue why. If we switched, we would need to update our pre-commit too I've just realized, and also everyone would need to update their IDEs (ugh).

Yeah, that seems cumbersome. I am having similar issues with another coding project where it seems like black does the formatting differently for the same version of black in combination with different python versions which lets me believe that the formatter is not so strict. But on the other hand, this causes the checks to fail so it is weird that it would be strict on the checking end of things while not being strict on the formatting itself. Let me have a more detailed look into this and find out if this is really the intended behavior.

@freemansw1
Copy link
Member Author

Okay, the specific issue it's cluing in on here is adding an extra comma after **kwargs, which was only supported in Python versions newer than 3.6 (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#trailing-commas). In code where it detects Python >3.6, it has already added them with simply black filename, but if it doesn't detect Python 3.6, it will not reformat. Adding the version specifiers adds the trailing comma as a requirement.

@freemansw1 freemansw1 mentioned this pull request Nov 8, 2023
8 tasks
.github/workflows/check_formatting.yml Outdated Show resolved Hide resolved
@JuliaKukulies
Copy link
Member

JuliaKukulies commented Nov 12, 2023

Looks good to me, thanks for these changes, @freemansw1!

@freemansw1 freemansw1 added this to the Version 1.5.2 milestone Nov 13, 2023
Copy link
Member

@w-k-jones w-k-jones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, happy for this to be merged

@freemansw1 freemansw1 merged commit b5e7f3f into tobac-project:RC_v1.5.x Nov 20, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants