feat: ✨ bring pre-commit and tests from supervision

[onuralpszr]

I guess headline pretty much tells everything, We already made successful in supervision. Maybe we can automate here to as well. No more manual linting if that is acceptable.

[onuralpszr]

This changes also require to lock python version to "py38" before merge this please consider this too. Saying "python 3" is very wide and 3.X(1-7) already EOL as well.

[capjamesg]

@onuralpszr Is there a way for you to submit this PR without all of the linting changes? I am happy to add a precommit, but this PR has a lot of merge conflicts in its current state.

[onuralpszr]

@onuralpszr Is there a way for you to submit this PR without all of the linting changes? I am happy to add a precommit, but this PR has a lot of merge conflicts in its current state.

Let me revision and ping you

[onuralpszr]

@capjamesg pre-commits are fixed and more tests added for make sure code is fine as well.

[capjamesg]

Looks good. I left one comment. It looks like this has formatted our long print statements to have multiple strings. Do the new strings appear as expected? My inline comment corresponds to one example.

[onuralpszr]

Looks good. I left one comment. It looks like this has formatted our long print statements to have multiple strings. Do the new strings appear as expected? My inline comment corresponds to one example.

That multiple string formatting not create "new line unless we add \n" so it is fine.

[capjamesg]

Thank you for the clarification. I have approved these changes and asked another team member to review, too.

[onuralpszr]

Thank you for the clarification. I have approved these changes and asked another team member to review, too.

Sure please do, I also want to fix colab bug as well. as soon as this one merge, I will fix other bugs too. (I am holding because I don't wanna create conflicts)

[onuralpszr]

@capjamesg also please add this repo to here (https://pre-commit.ci/) for auto fixes and CI actions you can do before merge or after merge but doing before is better so you can also see in action too.

[tonylampada]

@capjamesg @onuralpszr I went over the file changes and it really looks like this causes no change in runtime behaviour.
I still need to make some local tests anyway.
Also I need to have a better understanding of what the changes in pre-commit config and pyproject.toml mean for the dev env. I'm not too familiar with those tools so if any of you can give a more detailed explanation about that, it might speed me up a bit :-)

[onuralpszr]

Pre-commit is a framework for managing Git pre-commit hooks in a project. These hooks are scripts that run automatically before you commit any changes to your codebase. Their purpose is to catch potential issues in your code early on, preventing them from being pushed to the repository and potentially causing problems later.

Advantages of using pre-commit:

• Improved code quality: By running checks for formatting, linting, security vulnerabilities, and other issues, pre-commit helps ensure that your codebase is always clean and maintainable.

• Increased developer productivity: Catching issues early saves developers time and effort compared to discovering them later in the development process during code reviews or deployment.

• Enforced coding standards: Pre-commit allows you to define and enforce coding conventions for your project, leading to a more consistent and readable codebase.

• Reduced friction in collaboration: When everyone is working with the same coding standards and checks, it makes collaborating on code much smoother and less prone to conflicts.

• Language and technology agnostic: Pre-commit can be used with any programming language or technology, as long as the checks are available as pre-commit hooks. In our case we use for python and their tooling do automatic checks

Advantages of using pre-commit.ci website:

• Community-driven hooks: The website offers a library of pre-built hooks for various languages and tools, saving you time and effort in setting up your own checks.

• Continuous integration (CI) integration: pre-commit.ci can be integrated with your CI/CD pipeline to automatically run pre-commit checks on every pull request, further ensuring code quality before merging and commit to automatically in case of check failure and try to auto-fix but do only once in each commit so no endless loop.

Lastly we do use pre-commit heavily in supervision project and make sure codebase is in same standard.

Usage is also very easy

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

Install as git-hook locally in your clone repo

pre-commit install 

[onuralpszr]

Current run and result

pre-commit run --all-files

fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
check yaml...............................................................Passed
check docstring is first.................................................Passed
check that executables have shebangs.....................................Passed
check toml...............................................................Passed
check for case conflicts.................................................Passed
check for added large files..............................................Passed
detect private key.......................................................Passed
forbid new submodules................................(no files to check)Skipped
pretty format json.......................................................Passed
mixed line ending........................................................Passed
Sort imports.............................................................Passed
Flake8 Checks............................................................Passed
bandit...................................................................Passed
ruff.....................................................................Passed

[onuralpszr]

About pyproject.toml currently I use as for storing parameters of tooling we use like ruff , isort , bandit etc. And previously you already had toml for build-system

[build-system]
requires = ["setuptools>=57", "wheel"]
build-backend = "setuptools.build_meta"

I just added others config for pre-commit tools and it can automatically read from there

[tonylampada]

So if I understand correctly, the pre-commit change will have no effect for folks that don't have it installed then?

[onuralpszr]

So if I understand correctly, the pre-commit change will have no effect for folks that don't have it installed then?

For people who install "pip install roboflow" / users has no effect.(they don't need to install) But for devs and contributors has to install for meet the pre-commit requirements.

[tonylampada]

But for devs and contributors has to install for meet the pre-commit requirements.

Right. I'm just thinking abou what happens to the dev env of a a dev who just does "git pull" on this and doesn't know about it.
If I understand correctly, the pre-commit rules will never be enforced. Nothing changes in their dev env. Correct?

(and imo, at this time, "nothing changes" is exactly what we want: make it optional first, dogfood on it, and maybe later add it to the devenv setup with the devcontainer)

[onuralpszr]

But for devs and contributors has to install for meet the pre-commit requirements.

Right. I'm just thinking abou what happens to the dev env of a a dev who just does "git pull" on this and doesn't know about it. If I understand correctly, the pre-commit rules will never be enforced. Nothing changes in their dev env. Correct?

(and imo, at this time, "nothing changes" is exactly what we want: make it optional first, dogfood on it, and maybe later add it to the devenv setup with the devcontainer)

If person never install yes not gonna enforce it but "CI" will enforce it

[onuralpszr]

Right. I'm just thinking abou what happens to the dev env of a a dev who just does "git pull" on this and doesn't know about it.

I mean best we can do write in contribute guide and hopefully checks the toml and config files :) (I assume person is dev)

[onuralpszr]

(and imo, at this time, "nothing changes" is exactly what we want: make it optional first, dogfood on it, and maybe later add it to the devenv setup with the devcontainer)

For this change it needs enforce and add dev container upfront and run install hook so it will automatically will exist.

[tonylampada]

OK I understand now. This doesn't force any change in the dev env, commits will still work. and may get fixed automatically like it did here...

or the build will break if the code doesn't conform with the new rules.

This can go in as is. It's an improvement.
I'm thinking a good next step would be installing and configuring the pre-commit tool (along with helping vscode plugins, maybe) in the project's devcontainer. Then people using the devcontainer would have the advantage of avoiding the potential "code-bouncing" fight with the ci without even having to know the quirks of setting up the pre-commit tool locally. @onuralpszr if you want to do that in another PR I'll definitely take a look.