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

Minor fixes #9696

Closed
wants to merge 1 commit into from
Closed

Minor fixes #9696

wants to merge 1 commit into from

Conversation

ThomasLandauer
Copy link
Contributor

No description provided.

@greg0ire
Copy link
Member

greg0ire commented Apr 29, 2022

Please improve your commit message according to the contributing guide.

"Minor fixes" could be the commit message for a lot of commits.

Here, you should have 2 commits, one for each change:

use valid link syntax

There should be no leading slash.
Add missing use statement

To fix this:

git reset --mixed HEAD^
git add --patch (add one change)
git commit -v
git add --patch (add the other change)
git commit -v
git push --force

This was referenced Apr 29, 2022
@greg0ire
Copy link
Member

You could have just force pushed instead of creating 2 PRs :P

@greg0ire greg0ire closed this Apr 29, 2022
@ThomasLandauer
Copy link
Contributor Author

ThomasLandauer commented Apr 29, 2022

OK, I started from scratch :-) #9698 and #9699

Some projects prefer one combined PR for 10 minor fixes; you (obviously) prefer 10 separate. What I mean: Please don't be too strict here, since when contributing to multiple projects, it's impossible to remember who prefers what... ;-)

"Minor fixes" could be the commit message for a lot of commits.

Yes. On the other hand: It's kinda strange if the overhead (=writing commit message) takes way longer than the actual PR ;-)
Maybe the "solution" would be a separate repo for the docs (like Symfony) cause questions like "Why was this change made at all?" are important for the code, but (in many cases) just irrelevant for the docs.

You could have just force pushed instead of creating 2 PRs :P

Sorry, still a git beginner...

@ThomasLandauer ThomasLandauer deleted the patch-1 branch April 29, 2022 14:39
@greg0ire
Copy link
Member

Some projects prefer one combined PR for 10 minor fixes; you (obviously) prefer 10 separate.

Uuuuh no I don't. I prefer 10 commits in 1 PR. If the sum of all the commits stays reasonable of course.

What I mean: Please don't be too strict here, since when contributing to multiple projects, it's impossible to remember who prefers what... ;-)

I'll stay strict about commits if you don't mind, but you'll notice that I merged your separate PRs, so I'm not as strict as I could be already.

It's kinda strange if the overhead (=writing commit message) takes way longer than the actual PR ;-)

Yes it's kind of weird, and it happens when you have issues putting into words what happens in your head. It requires a bit of effort and time. But lucikly, this open source, meaning there are no deadlines, you can take all the time you need.

Maybe the "solution" would be a separate repo for the docs (like Symfony)

Nah, I don't think it was a great move on their part to do that. Makes it hard to enforce having documentation written for new features, and keeping documentation in sync IMO. https://principles.dev/p/documentation-should-be-close-to-the-code/

cause questions like "Why was this change made at all?" are important for the code, but (in many cases) just irrelevant for the docs.

Well about that first commit, until I noticed the links you contributed in that other PR a few weeks ago, I didn't know that the leading slash was just invalid syntax. So IMO it's worth mentioning that this syntax is plain wrong and that you are not merely doing a style tweak. Apparently it wasn't obvious for you or for me who reviewed that PR. Let's not assume it should be obvious for everyone then. Makes sense? Also doing the effort of taking a step back before writing a commit message is a great habit, it's a good idea to try to always do it, as if it were a reflex.

Overall, about strictness, this is:

  • open source software, with an evolving team of many contributors;
  • a package that has a lot of users.

So if you're going to contribute to it, you should IMO do your absolute best (better than anything you do professionally), in particular you should try hard to make any PR as understandable as possible, and expect maximum strictness because of the impact anything you do can have.

Here is a documentation PR I made 2 days ago, with 3 commits, each of which attempt to fully explain what was done with links to related commits and a body for each commit. I hope this helps.

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

Successfully merging this pull request may close these issues.

None yet

2 participants