[openstreetmap/openstreetmap-website] .github/workflows/lint.yml: Check if annotate_models is up to date (PR #4286)
Andy Allan
notifications at github.com
Wed Oct 18 13:39:32 UTC 2023
> I do not like rewriting the commit history in a PR with attached inline comments. It creates dangling comments and makes it harder to understand the PR history and structure retrospectively.
I think that it's fine to update an existing PR, despite the "dangling review comments" issue. But it's not a major deal. I think about 90% of the time it's better to update an existing PR.
> Why are Merge Commits the preferred way to merge and not squash-merging?
If we squash-merge then `git blame` only shows the whole PR, as if it was done in one large commit. This can also make it harder to run tools like `git bisect` to track down a problem, and it also means that individual component changes of a PR can't be explained in their corresponding commit messages.
So [we ask for contributors to](https://github.com/openstreetmap/openstreetmap-website/blob/3c706ab8470e3c638126abd765d3295ed543aea5/CONTRIBUTING.md#L104-L1050):
> * Split up large commits into smaller units of functionality.
> * Keep your commit messages relevant to the changes in each individual commit.
An example would be #4278 (Upgrade to rails 7.1) where there are a number of related-but-yet-distinct changes, and the commits explain each change in isolation. If we had just one squashed commit for that whole PR, it would be a nightmare in future to understand which code changes related to which part of the (consequently large, if you combined them all) commit message.
An alternative would be rebase-merging, where all the commits are kept but replayed/cherry-picked onto master. I'd be fine with that, if anyone things the merge commits themselves cause problems, but merge-commit-merges and rebase-merges are otherwise very similar.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4286#issuecomment-1768483030
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/4286/c1768483030 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20231018/bda98700/attachment.htm>
More information about the rails-dev
mailing list