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

feat(formatting): 🚀 pre-commmit,pre-commit config,import and code fixes #111

Closed
wants to merge 6 commits into from

Conversation

onuralpszr
Copy link
Contributor

@onuralpszr onuralpszr commented Sep 22, 2023

onuralpszr and others added 5 commits September 17, 2023 18:04
… req*.txt

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@fedoraproject.org>
@mabulgu
Copy link
Member

mabulgu commented Sep 22, 2023

Thanks for the pr @onuralpszr ! . Let me check pr #110 first (probably this weekend, sorry for the latency) and then checkout this one. I am a bit obsessed with formatting and I dont want to apply all the pep8 or other derived rules. Lets merge your first pr first and then I need to go over this one and we can discuss afterward.

Thanks for the great work!

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@onuralpszr
Copy link
Contributor Author

image

@mabulgu
Copy link
Member

mabulgu commented Sep 24, 2023

#77

@onuralpszr you mentioned you fix this one with this PR. Since the PR is big, before my detailed look at this (and #110 should come first) can I ask where and how exactly did you solve this. I am just asking not to miss anything.

@onuralpszr
Copy link
Contributor Author

onuralpszr commented Sep 24, 2023

#77

@onuralpszr you mentioned you fix this one with this PR. Since the PR is big, before my detailed look at this (and #110 should come first) can I ask where and how exactly did you solve this. I am just asking not to miss anything.

Merge #110 and if we change something I can rebase or fix conflict in here. But if you accept "as it is" well good because I create this PR from #110. Magical solve will happen :))

@mabulgu
Copy link
Member

mabulgu commented Sep 24, 2023

#77

@onuralpszr you mentioned you fix this one with this PR. Since the PR is big, before my detailed look at this (and #110 should come first) can I ask where and how exactly did you solve this. I am just asking not to miss anything.

Merge #110 and if we change something I can rebase or fix conflict in here. But if you accept "as it is" well good because I create this PR from #110. Magical solve will happen :))

Can you explain the magical solve for #77 if you dont mind? Not sure this one is fixed.

@mabulgu
Copy link
Member

mabulgu commented Sep 24, 2023

And yes #110 should be merged first as you have #110 in this one as well (so you will need a rebase for sure)

@onuralpszr
Copy link
Contributor Author

#77

@onuralpszr you mentioned you fix this one with this PR. Since the PR is big, before my detailed look at this (and #110 should come first) can I ask where and how exactly did you solve this. I am just asking not to miss anything.

Merge #110 and if we change something I can rebase or fix conflict in here. But if you accept "as it is" well good because I create this PR from #110. Magical solve will happen :))

Can you explain the magical solve for #77 if you dont mind? Not sure this one is fixed.

https://github.com/SystemCraftsman/strimzi-kafka-cli/pull/111/files#diff-6c5cdeeef00512ffa0304dbc3fb40ce1cd56276f4671b3d78994b5d8668a116aR12

no command gives you "help" but I guess you want "kfk command" <-- no param == shows you help ?

@mabulgu
Copy link
Member

mabulgu commented Oct 17, 2023

Hi @onuralpszr. Would you mind separating this PR into two like 1- pre-commit and other stuff like fstring usage etc. 2- formatting

So it will be a lot easier to review and discuss the changes.

Would you be able to do this? If not, pls let me know so I can work on this.

Copy link
Member

@mabulgu mabulgu left a comment

Choose a reason for hiding this comment

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

re: my comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants