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

Organize imports with platformdirs, fix flake8 #200

Closed
wants to merge 1 commit into from
Closed

Organize imports with platformdirs, fix flake8 #200

wants to merge 1 commit into from

Conversation

huakim
Copy link
Contributor

@huakim huakim commented May 24, 2024

No description provided.

@mcepl
Copy link
Contributor

mcepl commented May 26, 2024

Could you please stop generating zillion new pull requests? I agree that GitHub’s documentation is quite horrendous (there is something in https://docs.github.com/en/pull-requests/collaborating-with-pull-requests and for example just randomly https://www.dataschool.io/how-to-contribute-on-github/), but it is generally frowned upon to rebase and even more for killing PR and generating a new one. It is hard to compare two old PRs (I have to first start with hunting, which one closed PR it is), I have to basically review your whole diff again and again just to find out what did you change this time.

Do you want to understand this command which you forced me to develop?

 ~ $ interdiff \
    <(curl -L -s https://github.com/openSUSE/py2pack/pull/199.patch) \
    <(curl -L -s https://github.com/openSUSE/py2pack/pull/200.patch)|colordiff |less -r

Please, just add new commits to your branch, which you pushed in the first (and only) PR. If we want to squash all those commits into one, we can do it when merging.

Copy link
Contributor

@mcepl mcepl left a comment

Choose a reason for hiding this comment

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

still LGTM

@danigm
Copy link
Contributor

danigm commented May 27, 2024

And the commit message for this one is just "update", and there's no email in the Author metadata in commits...

I've done the merge manually and I've fixed the commit message, to avoid more steps for this simple fix.

But please, @huakim, take a look to the documentation provided by @mcepl , and try to use github and git correctly and provide as much information as possible. It will make easy the review process for everyone. It's easier to review the code when there's some kind of explanation about what it's in the PR and why in the PR description and/or commit message.

In any case, thank you for your work in py2pack, your contribution is very appreciated, but it always can be better 😄

@danigm danigm closed this May 27, 2024
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.

3 participants