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

ignore another big auto format #437

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Conversation

mikemhenry
Copy link
Contributor

@mikemhenry mikemhenry commented Dec 2, 2024

Tips

  • Comment "pre-commit.ci autofix" to have pre-commit.ci atomically format your PR.
    Since this will create a commit, it is best to make this comment when you are finished with your work.

Checklist

  • Added a news entry

Developers certificate of origin

@mikemhenry
Copy link
Contributor Author

Okay this also tests that the PR template is now working! We can access the release template with a quary URL param, I don't know where we want to put a link to that, maybe on our docs or wiki? This can be future work.

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (3cea95b) to head (f07a9b1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #437   +/-   ##
=======================================
  Coverage   98.67%   98.67%           
=======================================
  Files          36       36           
  Lines        2117     2117           
=======================================
  Hits         2089     2089           
  Misses         28       28           

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

@mikemhenry
Copy link
Contributor Author

Also I turned on "only squash commits" via the web UI since we talked about doing that but it never happened.

@ianmkenney can you check to see if when you run pre-commit locally now it doesn't try and change anything?

@ianmkenney
Copy link
Member

    cd `mktemp -d`
    git clone https://github.com/openfreeenergy/gufe && cd gufe
    micromamba create -p ../env/ python=3.11 pre-commit --yes
    micromamba -p ../env/ run pre-commit run --all-files

shows that no files were reformatted. Do we know why it took multiple calls of the bot to fix things? That sounds like a bug.

@mikemhenry
Copy link
Contributor Author

Do we know why it took multiple calls of the bot to fix things? That sounds like a bug.

I looked into it and looking at the logs (below) it had to do with an interaction where pyupgrade made a change (converted a .format into a f-string) that black didn't like (different line wrapping style), so black had to run again on those changes. For the last run nothing changed.

I will check to see if pyupgrade has any config options to play nicer with black.

image
image
image

@dotsdl
Copy link
Member

dotsdl commented Dec 4, 2024

@mikemhenry in discussion with @ianmkenney, can we just drop pyupgrade if it's fighting with black? We want to completely avoid nondeterminism in this system.

@mikemhenry
Copy link
Contributor Author

Sure I can update this PR with that change. Just a few notes:

  • Things are deterministic, even if the hooks were done in a different order, the results would be the same.
  • pyupgrade I think would be worth running when we bump the oldest python we support to a newer version, but that can be done manually in a separate PR as needed.

@dotsdl
Copy link
Member

dotsdl commented Dec 5, 2024

@mikemhenry works for me! We also have agreement from @atravitz, @jthorton, and @ianmkenney!

@mikemhenry mikemhenry requested review from dotsdl and jthorton December 5, 2024 16:37
@mikemhenry
Copy link
Contributor Author

Okay this should be good to go now!

@ianmkenney
Copy link
Member

lgtm, thanks @mikemhenry!

Copy link
Contributor

@jthorton jthorton left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikemhenry mikemhenry merged commit 4ee7647 into main Dec 6, 2024
19 checks passed
@mikemhenry mikemhenry deleted the maint/update-ignored-commits branch December 6, 2024 15:02
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.

5 participants