fsspec / community

Discussion space for conversations across multiple fsspec repos
5 stars 0 forks source link

merging strategy #5

Closed efiop closed 2 years ago

efiop commented 2 years ago

It seems like we currently use plain merging everywhere, which makes for a very messy commit history, where it is very common to have commits that break the test suit. Should we switch to squash-merging (the most hustle free, we get 1 commit in history, but all the granularity in the PR and don't have to enforce any practices) or rebasing (requires a lot of discipline and enforcement for contributors, but makes for the nicest linear history)? Also, should we adopt a common practice or let every repo decide? WDYT?

martindurant commented 2 years ago

Some of the repos are set to squash-merge already, so happy to go with this. I do not trust all developers (including myself) not to cause a mess with rebase! The only extra cost, mostly optional, of squash is to rewrite the merge message to trim useless commit messages and include anything relevant from the PR conversation.

isidentical commented 2 years ago

+1 to squash as well.

martindurant commented 2 years ago

As I recall, all it takes is to select "squash" the next time we want to merge, and that then will become the default in the future. Should we actually disallow simple merges, or might an exception be handy sometimes?

isidentical commented 2 years ago

It seems like we are still merging with the plain merge on fsspec/filesystem_spec and fsspec/s3fs. I wonder whether we can just make squash the default (uncheck other buttons from settings>options>merge button)?

martindurant commented 2 years ago

Unchecking would make plain merged impossible. I think it should default to whatever was done last time, so it's on me (or whoever clicks merge next) to remember to set it.

isidentical commented 2 years ago

Unchecking would make plain merged impossible

Though is there a use case for plain merge at all? Most of the projects I deal with already have this setting on, and I neve recall needing it (at least for merge, rebase might be actually useful on certain cases) 🤔

martindurant commented 2 years ago

I finally removed the simple merge as you suggest