Setup ci

[guergana]

No description provided.

[bereket-WMDE]

Hi @guergana
I'd suggest you squash the two commits together and submit the pull request again. The reason is to two folds:

1. to keep a clean git history
2. after working on a feature branch (experimenting and fixing bugs through multiple commits), there should just be one commit at the end for review. all the commit changes you've made before are just for you.

squash commits: git rebase -i HEAD~n : where n is the number of commits you want to squash.

I'm open to suggestions, counter-points and feedbacks

[jakobw]

I'd be happy with the commits as they are. I'm also a fan of a clean commit history, but in this case I'd argue that each of the commits provide value on their own. Introducing npm-run-all for the npm run test command is arguably also an improvement on developer ergonomics when testing the code locally, and does not purely exist to power the CI job. They're definitely related though, so I'm not opposing squashing either.

[guergana]

I agree with Jakob, I think they are separate tasks, but could also squash. Don't have a strong opinion about this.

[bereket-WMDE]

It's up for a vote ofc.
but I'm very much squash pro :)

think of this scenario. you're working on a feature for a couple of days and this is your commit history

final working commit
fix feedback after code review
more typo fix
oops forgot a package
fix typo
initial commit

do we really want this ?
but like i said, If the vote is 2 -1, I'll accept it

[jakobw]

@bereket-WMDE with the example history you provided it's a no-brainer that the commits should be squashed. It becomes a different story if the individual commits are actually meaningful on their own.

Contrived example: Imagine you weren't part of our pairing session and you got used to running npm run lint and suddenly that command is gone (which is what happened in this PR). If you had to dig this up in the history, would you rather see one commit that says Set up CI with this detail hidden in it, or would it be nicer to see Set up CI, Combine test and linting commands under test:*?

TL;DR do I think that we sometimes want multiple commits in one PR? Yes! Do we need two commits in this case? Yyy... maybe? No big deal either way IMO.

[bereket-WMDE]

@jakobw
you make a good point.
@guergana lets then go ahead and merge as it is.