Pokeapi: Merging and linting strategies

[tdmalone]

Our two main repositories are now pokeapi and ditto.

To reduce confusion for maintainers, it would probably be a good idea to standardise management of these repos, so I was looking at what is currently set up differently between the two. One item is the allowed merge strategies:

• pokeapi currently allows rebase merging only
• ditto currently allows squash merging only

The remaining option is merge commits.

Does anyone have any concerns or thoughts one way or the other about how we should manage merging? I personally like merge commits, dislike squash merging, and am indifferent about rebase merging - but in the end I don't really mind what we go with; I just love consistency 😆

Does anyone have any strongly held opinions on this, or otherwise any objections to changing ditto to the same merge strategy as pokeapi, now that it is a part of the PokeAPI organisation? Ping @sargunv as the primary maintainer of ditto. 😄

(Worth noting it's also possible to allow multiple merge strategies - although I think it might be better to stick to one)

[sargunv]

I generally dislike merge commits because the diff is duplicated across two commits (the real one and the merge one) in the commit history, and it pollutes the history a bit with extra commits.

I'm happy with either rebase or squash commits, but I prefer squash so incremental changes in a PR (say, after a review) don't pollute the history.

Should we standardize linting across pokeapi and ditto too? My personal preference here is black for formatting with flake8 for linting, configured to ignore PEP8 formatting (W and E error classes) in favor of black.

I've also been experimenting with Stickler-CI to enforce black and flake8 on PRs in ditto. Overall I like the concept, since it'll automatically apply black and update the PR instead of commenting, but it's also been a bit buggy (example: https://github.com/PokeAPI/ditto/pull/17). I may just use a more traditional CI solution to enforce lints (something I plan on doing anyway to enforce tests).

[tdmalone]

I've edited the scope of this issue to cover linting as well - definitely would be good to standardise that as well. I haven't used Stickler before but I love the idea of it - if it works that is, haha.

We currently use flake8 for the main PokeAPI repo; if we added black, would this involve significant reformatting to comply?

[sargunv]

Black's formatting is mostly similar, the main difference is 10% longer line length and lack of configuration options (which I see as a positive). I can open a PR with black applied to get an idea of the change.

Apart from the line length difference, I think black is actually more compliant with PEP than flake8.

[sargunv]

Apart from the migrations directories, looks like the change is mostly string normalization and line wrapping.

[neverendingqs]

TL;DR: I'm okay with any of the options.

Squash and merge (and merge commit) is nice because it automatically includes the Pull Request number in the commit, which is nice when trying to figure out past historical decisions. Rebase and merge does not (and can not really unless a commit gets modified).

However, if I recall correctly, squash and merge only gives credit to the person hitting the merge button. In other words, if multiple people submit commits to the same branch, I believe only the person hitting merge will have their name on the commit.

Merge commit does not have either shortcomings, but as @sargunv said makes the history a bit messier. Rebase and merge can also cause a messy history if there are a lot of "minor" commits on the branch at time of merging (also as @sargunv said earlier).

I am personally a fan of squash and commit (cleaner history), but would hate to not give proper credit, in which case I lean towards merge commits so the commit includes the Pull Request number.

I am also a newcomer, so I'm happy to side with what more tenured / active contributors want :).

[sargunv]

@neverendingqs It shows something like "Foo authored and Bar committed" in GitHub. Not sure about the underlying Git commit representation; I would assume it just lists multiple authors.

[lmerotta]

Personnally, I prefer the squash strategy, much cleaner than merge commit. I have no special opinion on rebase too, but having used squash a lot, I'm more familiar with it.

[tdmalone]

Not sure about the underlying Git commit representation; I would assume it just lists multiple authors.

It's the same as how GitHub shows it - git has the concept of both an author and a committer.

Aside from that GitHub/git _might_ add multiple authors if multiple authors' commits have been squashed into one? I haven't tested that.

[Naramsim]

I don't really like squashing. It is cleaner but you lose information

[lmerotta]

Well, since the PokéAPI project is older than ditto in this org, can I suggest to keep rebase merging, and change ditto's strategy to rebase ?

[sargunv]

@lmerotta I've already updated the ditto repo to allow squash or rebase, so we can pick whatever makes sense for the PR. I'd rather have squash available while I'm rapidly iterating on it, so I can move fast and not think about clean commit messages until merge time.

[tdmalone]

^ That's probably a good way to get the best of both worlds. Allow both squash and rebase, and try to make the best decision at merge time to avoid losing too much of the context on a big PR.

If this comment, or the previous one from @sargunv, gets a few positive reactions and no negative ones, I'll make the change to the pokeapi repository as well.