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

Adiabatically turn on pre-commit #678

Merged
merged 13 commits into from Nov 29, 2022
Merged

Conversation

jmduarte
Copy link
Member

@jmduarte jmduarte commented Nov 2, 2022

Adiabatically turn on pre-commit. The idea is that pre-commit is only run on files touched in a PR. Simultaneously we can open PRs to run pre-commit on files not often updated (or that just aren't touched by any open PRs). Eventually, we can run it on the whole repo.

Changes

  • Add pre-commit config
  • Add clang-format config (@vloncar which style do you prefer? This is basically LLVM style stolen from scikit-hep/boost-historgram)
  • Use p-clang-format to keep pragmas indented at the right level
  • Add pre-commit GitHub action that should only operate on files touched in the PR

Note the con that this may make it harder to review PRs (as a lot of changes will just be style). To be discussed if that's an issue.

To see what it would look like if we ran this on the whole repo in one go, you can check out this branch and do (on Linux):

pre-commit install 
pre-commit run --all-files

I say on Linux, because there seems to be some sed issue with OSX.

@jmduarte jmduarte mentioned this pull request Nov 2, 2022
@jmduarte jmduarte changed the title pre-commit Adiabatically turn on pre-commit Nov 2, 2022
@jmduarte jmduarte requested a review from vloncar November 2, 2022 01:34
@jmduarte jmduarte self-assigned this Nov 2, 2022
@jmduarte
Copy link
Member Author

@vloncar what do you think about this approach?

@vloncar
Copy link
Contributor

vloncar commented Nov 15, 2022

I think this is great. I still didn't get around to check how does the formatted C++ look like.

I guess we should also note in the documentation about contributing that users are advised to run pre-commit before making PRs, it will save us a fixing commits on many new PRs.

@jmduarte
Copy link
Member Author

jmduarte commented Nov 17, 2022

@vloncar to see the diff if we do pre-commit --run all on this PR you can look here: jmduarte/hls4ml@pre-commit...jmduarte:hls4ml:pre-commit-run-all

Note there are still some manual things that we would need to fix (mostly flake8 violations), but they're all pretty trivial.

@vloncar
Copy link
Contributor

vloncar commented Nov 17, 2022

This looks great! Is there any way to keep some of the current conventions? I understand black's preference, but the codebase uses single quotes everywhere in python, so majority of the changes are related to that. So maybe we add --skip-string-normalization? I have a slight preference for that. What do others think?

In HLS code, can we use 4 spaces instead of two (IndentWidth) and the line limit (ColumnLimit) that matches the Python one (125)? The rest of the C++ formatting looks ok.

@jmduarte
Copy link
Member Author

@vloncar updated as you requested and new diff is here: jmduarte/hls4ml@pre-commit...jmduarte:hls4ml:pre-commit-run-all

@jmduarte jmduarte added the please test Trigger testing by creating local PR branch label Nov 17, 2022
@vloncar
Copy link
Contributor

vloncar commented Nov 17, 2022

Thanks, I'm liking this one a lot. Is there a way to exclude some C++ files? I'm thinking the contents of ap_fixed and ac_fixed. These are (mostly) copied from the vendors, perhaps we should keep them intact, as it would be easier to track any future changes to them.

@jmduarte
Copy link
Member Author

yes, let me add that. i was thinking the same.

@jmduarte jmduarte removed the please test Trigger testing by creating local PR branch label Nov 17, 2022
@jmduarte jmduarte added the please test Trigger testing by creating local PR branch label Nov 17, 2022
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 17, 2022
@vloncar
Copy link
Contributor

vloncar commented Nov 22, 2022

Anyone else with their style preferences? This is the perfect bikeshedding PR 😄 If not, I propose we merge it.

@jmduarte
Copy link
Member Author

jmduarte commented Nov 28, 2022

@vloncar no complaints, so I say we merge 😄

@vloncar
Copy link
Contributor

vloncar commented Nov 29, 2022

And now, the avalanche of changes begins 🏔️

@vloncar vloncar merged commit 2010aa2 into fastmachinelearning:main Nov 29, 2022
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
* pre-commit

* update p-clang-format

* update p-clang-format

* update manifest

* update manifest

* add citation

* update format

* add null clang formats

* file-level

* style file

* global exclude

* try without file-level clang-format

* add pre-commit to PR template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants