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/update headers for all active files, add pre-commit #142

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

voetberg
Copy link
Contributor

@voetberg voetberg commented Aug 2, 2024

Addresses #141

Looks huge but it's just sorting imports according to the ruff settings from the main repo and adding headers to everything except probes in the attic. Doesn't address any of the concerns the pre-commits (either flake8 or ruff) bring up.

@voetberg voetberg added the enhancement New feature or request label Aug 2, 2024
@rdimaio
Copy link

rdimaio commented Aug 5, 2024

There's a few conflicts, please rebase

Copy link
Contributor

@dchristidis dchristidis left a comment

Choose a reason for hiding this comment

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

There are many more shortcomings, like not handling existing licences in triple-quoted strings, removing useful comments, not treating utils as a local import, bundling other smaller fixes together in the same commit.

Plus I’m concerned about having to maintain the same files across different repositories.

@voetberg
Copy link
Contributor Author

voetberg commented Aug 5, 2024

There are many more shortcomings, like not handling existing licences in triple-quoted strings, removing useful comments, not treating utils as a local import, bundling other smaller fixes together in the same commit.

These are all fixable, no problem.

Plus I’m concerned about having to maintain the same files across different repositories.

We can pull just the 'add_header' script from the main repo if we don't want to bother with maintaining that file in particular. As for the ruff toml, it's different enough that I think it's worth the work. Same goes for a pre-commit, but I'm a sucker for automation.

Download the add-header script from the main repo, apply it to the probes outside of the attic
Add ruff config that follows the main repo requirements, but includes utils as a local folder import
Add a pre-commit that matches the main repo requirements. Runs the add-header shell script instead of the py version
* Rucio is a first party, utils is a local folder.
@voetberg voetberg force-pushed the pre_commit_and_header branch from be8ac5b to 8dd1980 Compare August 6, 2024 16:36
@voetberg voetberg requested a review from dchristidis August 6, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants