SystemCraftsman / strimzi-kafka-cli

Command Line Interface for the Strimzi Kafka Operator
Apache License 2.0
78 stars 13 forks source link

feat: 🚀 introduce pyproject.toml file and remove old setup.py/cfg and req*.txt #110

Closed onuralpszr closed 11 months ago

onuralpszr commented 12 months ago

Hello, I see that project was using old setup.py style and I wanted to bring new toml style and also you can set "formatting rules" inside and next if this one accepted I would like to introduce pre-commit.ci so all of the formatters will be automatic and git-hook possible. (easy to attach thanks to pre-commit CLI)

For produce wheel/install/development I am using "poetry" for control toml file

Links: https://pre-commit.ci/ https://python-poetry.org/ https://github.com/pre-commit/pre-commit

mabulgu commented 12 months ago

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon.

BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

onuralpszr commented 12 months ago

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon.

BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

You're welcome, I am glad you like it. With these PR I can fix #104 #105 #106 very easily and also can introduce "ruff" as well.

mabulgu commented 12 months ago

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon. BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

You're welcome, I am glad you like it. With these PR I can fix #104 #105 #106 very easily and also can introduce "ruff" as well.

Nice! I had a draft trial PR here for that stuff (but without pyproject.toml which I realized its existence a lot later on) so your PR will be a good base for this one.

I see that your builds are failing. Can you check before I review your changes?

onuralpszr commented 12 months ago

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon. BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

You're welcome, I am glad you like it. With these PR I can fix #104 #105 #106 very easily and also can introduce "ruff" as well.

Nice! I had a draft trial PR here for that stuff (but without pyproject.toml which I realized its existence a lot later on) so your PR will be a good base for this one.

I see that your builds are failing. Can you check before I review your changes?

Sure, I am checking CIs right now.

onuralpszr commented 12 months ago

@mabulgu run the CI please :)

mabulgu commented 12 months ago

@mabulgu run the CI please :)

Done. Also removed the silly rule for approval. Pls ping me if it pops up again tho, GH settings are generally complex.

onuralpszr commented 12 months ago

@mabulgu run the CI please :)

Done. Also removed the silly rule for approval. Pls ping me if it pops up again tho, GH settings are generally complex.

LOL , Sure I will ping you, let me see what is complaining

onuralpszr commented 12 months ago

@mabulgu oh btw, python 3.7 is EOL ? Can we drop it ? or do you really need it ?

For example isort asking for 3.8 minimum and so many other things not gonna be easy to install for py37

onuralpszr commented 12 months ago

@mabulgu, I passed all the tests, but the 'click' behavior was causing the issue because the latest one has a different exit code. So, I had to revert for now, but it needs to be changed in the tests to comply with that. I thought I was missing something for a while, but then I understood that 'click' was the issue.

onuralpszr commented 11 months ago

@mabulgu ready for review and dropped python 3.7. Because too many dev deps needs to be adjusted and too many projects also dropped as well. I did not apply any formatting yet. If you want me do that, I can change all files for formatted versions but I don't wanna mix too commits with it.

PS : I wanted to say "friendly hello" with this way in PR :) Have a nice day 🚀

onuralpszr commented 11 months ago

@mabulgu if we going to control ALL of the formatting and etc via "pre-commit" for saving time in CI we can remove "flake" section, because pre-commit ci gonna be different CI and it is "soooo much faster" like 1-2 seconds and done. that means for CI test we can remove "poetry" and use "pip install ." plus install twine because twine is dev tool. This should be more minimalist approach

onuralpszr commented 11 months ago

@mabulgu I added cache for poetry so install time just become "1 second" as long as we change nothing on deps "cache" will stay which is a good thing because CI can work faster. Plus if we turn off flake in action (which I really really want to not use flake8 in action) I can speed so much more as well. for now top times now become 25-30 (previous was extra 10-15 seconds longer) I also updated gh-action versions for better support which handle warnings (check your old CIs you will see what I meant)

mabulgu commented 11 months ago

Probably I explained it in a wrong way. It is not only about timing here. It is just because of the YAGNI principle. If you really dont need sth you should not be using it so it was what I thought about venv. The timing was just a small side effect, nothing else.

Re: poetry cache - thanks for it, I think it will be useful

Re: flake check - even if we use it we should be checking stuff there as a validation because pre-commit can be skipped by uninstalling and anybody can commit a code without those checks. Lets keep it for now.

onuralpszr commented 11 months ago

Re: flake check - even if we use it we should be checking stuff there as a validation because pre-commit can be skipped by uninstalling and anybody can commit a code without those checks. Lets keep it for now.

no you can't skip that.
Example repo with pre-commit (I setup before) https://github.com/roboflow/supervision/commits?author=pre-commit-ci%5Bbot%5D

There will be "bot" will handle that.

onuralpszr commented 11 months ago

Probably I explained it in a wrong way. It is not only about timing here. It is just because of the YAGNI principle. If you really dont need sth you should not be using it so it was what I thought about venv. The timing was just a small side effect, nothing else.

Re: poetry cache - thanks for it, I think it will be useful

Sure, I understand that but I made it better :)

mabulgu commented 11 months ago

Re: flake check - even if we use it we should be checking stuff there as a validation because pre-commit can be skipped by uninstalling and anybody can commit a code without those checks. Lets keep it for now.

no you can't skip that. Example repo with pre-commit (I setup before) https://github.com/roboflow/supervision/commits?author=pre-commit-ci%5Bbot%5D

There will be "bot" will handle that.

I think I missed this commetn of yours, sorry for that. When you uninstall precommit or not install it, you can simply skip it and commit things in the repo. So since it depends on the self-responsibility of the developer and how he/she reads the contributor guide, an extra direct check always is good.

mabulgu commented 11 months ago

@onuralpszr after doing the following I think we can merge this PR:

Make will be used both ci/cd and dev process so it is important to reflect formatting, linting, building etc. here.

Thanks!

If you have any doubts you cannot do the reverting to setuptools because of time concerns pls let me know. I can take over your pr.

My best

Regarding our conversation on linkedin: you said that you already applied the rules in my legacy draft pr, so pls skip that.

re: additions in the makefile - I can gather up the useful commands in the makefile later on so thats on me.

If you have doubts about removing poetry and using setuptools, pls let me know shortly so I can take this over and work on the required changes. Thanks!

onuralpszr commented 11 months ago

@mabulgu it is now setuptools with proper CI with dev tools and pip cache for speed up.

Minor request if possible, can you make this PR labeled "hacktoberfest-accepted" but in order to for that you need to add topic "hacktoberfest" into project page (you can click gear on next to so it will count as PR in HB event for me too.

onuralpszr commented 11 months ago

@mabulgu friendly ping* :)

mabulgu commented 11 months ago

@mabulgu friendly ping* :)

I will re-check the PR as soon as possible. I have been quite busy recently so you need to bear with me a bit. Thanks for your patience!