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

add active PRs to black ignore #4815

Merged
merged 8 commits into from
Dec 19, 2024

Conversation

marinegor
Copy link
Contributor

@marinegor marinegor commented Dec 2, 2024

Fixes #

Changes made in this Pull Request:

  • add files present in open PRs to the list of extend-exclude for black

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4815.org.readthedocs.build/en/4815/

@marinegor
Copy link
Contributor Author

cc @RMeli -- as we discussed, I finally managed to automate creation of the list of files in active PRs. Not sure if it works as necessary (e.g. the comment lines in the list worry me, and I assume that everything after the first comment is not accounted for) -- could you double-check if it works for you? Or perhaps there's a better way to configure these files?

@marinegor
Copy link
Contributor Author

The code I used to generate the lists:

#!/bin/bash

echo '' > active_files.lst
for num in $(gh pr list -L 200 | awk '{print $1}'); do
  echo Working on $num...
  echo "# ${num} $(gh pr view ${num} --json title --jq .title)" >> active_files.lst
  gh pr diff  --name-only ${num} >> active_files.lst
  echo '# ---------------------' >> active_files.lst
done

cat active_files.lst | grep -E '^(#|package/.*.py)' > active_files_package.lst
cat active_files.lst | grep -E '^(#|testuite/.*.py)' > active_files_testsuite.lst

@RMeli
Copy link
Member

RMeli commented Dec 4, 2024

Thanks @marinegor, looks useful! We will have to fix CI first, and merge the lib formatting which is already mainly done.

@RMeli
Copy link
Member

RMeli commented Dec 6, 2024

@marinegor I had a look at your script. I think looking at 200 PRs is a bit pessimistic. I think it might be worth including only PRs that have been updated (JSON filed: updatedAt) in the last few months. Maybe 2 months is enough (or even too much)? If a PR has been stale for a while, there will be likely conflicts anyways and we can help on a case-by-case basis.

@marinegor
Copy link
Contributor Author

@RMeli 2 months it is, updated

@RMeli
Copy link
Member

RMeli commented Dec 8, 2024

Thanks @marinegor, can you please share the script to filter by updatedAt? (Sorry, I'm being lazy here...)

@RMeli
Copy link
Member

RMeli commented Dec 8, 2024

Please also fix the conflicts and exclude the files of the bits that we already formatted (lib/ for instance).

@marinegor
Copy link
Contributor Author

@RMeli the script:

#!/bin/bash

rm active_files.lst
for num in \
  $(gh pr list -L 200 --json number,updatedAt,createdAt,title \
  | jq '[.[] | select(.updatedAt > ("2024-10-02"))]' \
  | jq '.[] | .number');
do
  echo Working on $num...
  echo "# ${num} $(gh pr view ${num} --json title --jq .title)" >> active_files.lst
  gh pr diff  --name-only ${num} >> active_files.lst
  echo '# ---------------------' >> active_files.lst
done

cat active_files.lst | grep -E '^(#|package/.*.py)' > active_files_package.lst
cat active_files.lst | grep -E '^(#|testsuite/.*.py)' > active_files_testsuite.lst

and then I manually move the lists to pyproject.toml's

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.63%. Comparing base (a27591c) to head (e3a98e4).
Report is 9 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4815      +/-   ##
===========================================
- Coverage    93.68%   93.63%   -0.05%     
===========================================
  Files          177      189      +12     
  Lines        21743    22845    +1102     
  Branches      3055     3064       +9     
===========================================
+ Hits         20370    21391    +1021     
- Misses         927     1002      +75     
- Partials       446      452       +6     

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

@RMeli
Copy link
Member

RMeli commented Dec 10, 2024

As discussed I closed #4518. The situation looks much better.

PRs that still do a reasonable amount of changes are

#4810 should be easy to fix and incorporate quickly. But I think if you can update the exclusion list, we can merge this and start formatting what is left.

@RMeli
Copy link
Member

RMeli commented Dec 10, 2024

Please do save the active_files_*.lst files, so that we have the list of PRs to keep track of and we can do the formatting as soon as a PR is merged.

@RMeli
Copy link
Member

RMeli commented Dec 18, 2024

@marinegor any chance this can be updated in the next few days? I plan to work on the reminder of the code base during Christmas break.

@marinegor
Copy link
Contributor Author

@RMeli sure, just ping me like 48 hours in advance, top.

Also, I could add the script to the maintainer directory of the package, though don't really think it's a good idea -- but ping me if it is.

@RMeli
Copy link
Member

RMeli commented Dec 18, 2024

I think about now is more and less 48 hours in advance to my holidays 😛

I think the script is very specific for this transition, so it is good to have it in the PR comments, but I don't think there is a real need to have it within the repo.

@marinegor
Copy link
Contributor Author

marinegor commented Dec 18, 2024

@RMeli done!

I meant that perhaps it's good to not do that again if a PR gets updated, but then I just decided to update the script:

#!/bin/bash

rm active_files.lst
for num in \
  $(gh pr list -L 200 --json number,updatedAt,createdAt,title \
  | jq '[.[] | select(.updatedAt > ("2024-10-02"))]' \
  | jq '.[] | .number');
do
  echo Working on $num...
  echo "# ${num} $(gh pr view ${num} --json title --jq .title)" >> active_files.lst
  gh pr diff  --name-only ${num} >> active_files.lst
  echo '# ---------------------' >> active_files.lst
done


cat active_files.lst | sed '/#/d' | sed -e 's/\.py$/\\.py/' | sed -e 's/^\(.*\)\(\n\1\)\+$/\1/' | grep -E '^(#|package/.*.py)' | sed '1!s/^/| /' > active_files_package.lst
cat active_files.lst | sed '/#/d' | sed -e 's/\.py$/\\.py/' | sed -e 's/^\(.*\)\(\n\1\)\+$/\1/' | grep -E '^(#|testsuite/.*.py)' | sed '1!s/^/| /' > active_files_testsuite.lst

now the files that it outputs can be directly pasted after the __pycache__ line in the respective pyproject.toml file.

@RMeli
Copy link
Member

RMeli commented Dec 19, 2024

Thanks @marinegor, I'm going to merge this. The files are useful so that we can remove the different exclusions when PRs are merged. I'll make a note to remove them once formatting is completed.

@RMeli RMeli enabled auto-merge (squash) December 19, 2024 20:57
@RMeli RMeli merged commit 4f143fd into MDAnalysis:develop Dec 19, 2024
23 of 24 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.

2 participants