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 pre-commit CI action #4982

Merged
merged 17 commits into from
Oct 28, 2021
Merged

Add pre-commit CI action #4982

merged 17 commits into from
Oct 28, 2021

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Sep 28, 2021

this helps to maintain code formatting 🐰
moreover you may consider add this bot - https://github.com/marketplace/pre-commit-ci

πŸ› οΈ PR Summary

Made with ❀️ by Ultralytics Actions

🌟 Summary

Introduce code formatting workflows and update minor comment syntax.

πŸ“Š Key Changes

  • Added a code formatting workflow using flake8 and pre-commit.
  • Minor white-space and code clean-up in various files for better syntax adherence.
  • Correction of typos and removal of trailing whitespace in documentation files.
  • Ensured consistency in Python scripts by using comprehensive list comprehensions and generators.

🎯 Purpose & Impact

  • πŸš€ Improves code readability and maintainability with automated styling checks.
  • 🐞 Helps prevent syntax errors and facilitates future debugging.
  • πŸ’… Promotes a consistent coding style throughout the project, improving collaboration.

@Borda Borda force-pushed the pre-commit branch 3 times, most recently from 15867b7 to 202f898 Compare September 29, 2021 15:41
@Borda
Copy link
Contributor Author

Borda commented Sep 29, 2021

@glenn-jocher mind have look? 🐰

@glenn-jocher
Copy link
Member

@Borda thanks! Is there any way to move the new .yaml and .cfg files from the top level of the repo into a subdirectory?

@Borda
Copy link
Contributor Author

Borda commented Sep 30, 2021

@Borda thanks! Is there any way to move the new .yaml and .cfg files from the top level of the repo into a subdirectory?

I am not aware of it... do you have a reason not to have it in the root?

@glenn-jocher
Copy link
Member

glenn-jocher commented Oct 2, 2021

@Borda well that's a good question. I'm not a design or UX expert, but my main intention with the repo is to make everything as simple and easy as possible. I'd like to keep the root directory minimal to not scare away first time users (and hopefully to have each file in the root have an easily identifiable purpose at first glance). A minimal root with also increase the chances that the README banner will appear on page load without users having to scroll down too much, which may increase conversions (from viewers to >> cloners).

But I want to add linting at the same time! :)

In addition to these two PRs maybe we could set up another meeting so we could discuss the high level changes we should be pursuing, and then we can work out the details for these two PRs?

@Borda
Copy link
Contributor Author

Borda commented Oct 3, 2021

my main intention with the repo is to make everything as simple and easy as possible

well yes but if you want to have some settings in other folders instead of default root, you will add extra complexity instead and hope that this re-routing will work for everyone

keep the root directory minimal to not scare away first time users

Check for example sklearn - https://github.com/scikit-learn/scikit-learn how their root looks like
also do not see why it may scare anyone, I would be rather scared if there are too few things :D

A minimal root with also increase the chances that the README banner will appear on page load without users having to scroll down too much, which may increase conversions

I feel that is a kind of misunderstanding of the repo readme, if you want a landing page from readme GH can give it for you

In addition to these two PRs maybe we could set up another meeting so we could discuss the high level changes we should be pursuing, and then we can work out the details for these two PRs?

fine with me, also we can sen some "refactoring" GH project to fill tickets with expected work so also other contributors can help

@Borda
Copy link
Contributor Author

Borda commented Oct 3, 2021

@glenn-jocher Anyway, I would merge this one as entry gate if you agree...

@Borda
Copy link
Contributor Author

Borda commented Oct 13, 2021

@glenn-jocher can we merge this one?

@Borda
Copy link
Contributor Author

Borda commented Oct 21, 2021

@glenn-jocher not updating it anymore until you ask for it just before merging 🐰

@glenn-jocher
Copy link
Member

@Borda hi there! Yes I'd like to get this merged. I see you've kept it up to date with master, thank you!!

Is there really no way to tidy up the root directory a bit? Can we shift any of the new files into .github or .github/workflows?

@glenn-jocher
Copy link
Member

glenn-jocher commented Oct 22, 2021

Oh BTW I implemented some updates to the rebase action (dependabot now updates actions, so dependabot updated to rebase.yml to v1.5, and I also replaced GITHUB_TOKEN with a new ACTIONS_TOKEN personal access token that includes workflows permissions), so it might be possible to rebase using the /rebase command now, but it still doesn't work for many PRs unfortunately.

EDIT: nope, seems like still failing with GitHub doesn't think that the PR is rebaseable! error
https://github.com/ultralytics/yolov5/runs/3979145773?check_suite_focus=true

@Borda
Copy link
Contributor Author

Borda commented Oct 22, 2021

Is there really no way to tidy up the root directory a bit? Can we shift any of the new files into .github or .github/workflows?

not really as pre-commit is an independent tool, not GH util
well, you can move the CONTRIBUTING.mb to .github

@glenn-jocher glenn-jocher added the TODO High priority items label Oct 23, 2021
@glenn-jocher
Copy link
Member

glenn-jocher commented Oct 23, 2021

@Borda could you add a header to each of the 3 new files that explains what they do in a sentence or two? I think this would help everyone understand them better if users decide to open them and look around.

I see that pre-commit requires .pre-commit-config.yaml as they say in https://github.com/marketplace/pre-commit-ci, but they also say that nothing else is required beyond that. I'm guessing setup.cfg is required by Flake8 which is called by pre-commit?

Then the last file I'm a bit confused about, code-format.yml. Is this what actually runs pre-commit on PR commits?

BTW I think code-format.yml may have a problem, as it has branches: [main], but we are using master branch as the main branch, we haven't migrated over to the new naming.

@Borda
Copy link
Contributor Author

Borda commented Oct 26, 2021

I see that pre-commit requires .pre-commit-config.yaml as they say in https://github.com/marketplace/pre-commit-ci, but they also say that nothing else is required beyond that.

yes

I'm guessing setup.cfg is required by Flake8 which is called by pre-commit?

exactly

Then the last file I'm a bit confused about, code-format.yml. Is this what actually runs pre-commit on PR commits?

correct or you can activate this bot and then the new action is not needed, https://github.com/marketplace/pre-commit-ci

BTW I think code-format.yml may have a problem, as it has branches: [main], but we are using master branch as the main branch, we haven't migrated over to the new naming.

@glenn-jocher fixing it now

@glenn-jocher glenn-jocher changed the title add pre-commit Add pre-commit CI action Oct 28, 2021
@glenn-jocher glenn-jocher self-requested a review October 28, 2021 16:34
@glenn-jocher glenn-jocher merged commit ed887b5 into ultralytics:master Oct 28, 2021
@glenn-jocher
Copy link
Member

@Borda PR is merged. Thank you for your contributions to YOLOv5 πŸš€ and Vision AI ⭐

Do you think we should add the bot at https://github.com/marketplace/pre-commit-ci, or should we wait a bit and see how this PR works?

@glenn-jocher glenn-jocher removed the TODO High priority items label Oct 28, 2021
@Borda Borda deleted the pre-commit branch October 28, 2021 16:57
@Borda
Copy link
Contributor Author

Borda commented Oct 28, 2021

Do you think we should add the bot at https://github.com/marketplace/pre-commit-ci, or should we wait a bit and see how this PR works?

I would rather use the bot as it pushes fixes to open PRs automatically even to forked PRs, compare to actions that works only with own PRs 🐰

BjarneKuehl pushed a commit to fhkiel-mlaip/yolov5 that referenced this pull request Aug 26, 2022
* define pre-commit

* add CI code

* configure

* apply pre-commit

* fstring

* apply MD

* pre-commit

* Update torch_utils.py

* Update print strings

* notes

* Cleanup code-format.yml

* Update setup.cfg

* Update .pre-commit-config.yaml

Co-authored-by: Glenn Jocher <[email protected]>
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