Run linting during CI (and other small cleanups) #477
Conversation
Otherwise it tends to break over time.
regPC cannot be undefined at this point because we assert as much above, so the non-null assertions are not needed. However the TypeScript compiler does not know that and would error out because it does not understand `expect(regPC).to.exist`. It does understand `assert(regPC !== undefined)`, so replace it with that. (Not an exact replacement because `exist` also checks for null, but that cannot occur here.)
To catch accidentally committed `.only(` on tests.
jonahgraham
left a comment
jonahgraham
left a comment
There was a problem hiding this comment.
thanks for addressing these items - feel free to merge once you are satisfied
|
Is there a strict policy on squashing/rebasing/non-ff-merging pull request branches? From what I have seen in this project so far, it seems like they are always squashed (so that’s what I did here too). Personally, I am not a fan of doing that unconditionally, because sometimes the separation into multiple commits contains important information that is lost in squashing. In the case of this PR, I would probably have left the commits separate because they address independent concerns (but rebased on the target branch to keep a linear history). But I am okay with squashing too, because these commits touch disjoint sets of files, so the squashed commit is easy to mentally take apart again. On some of my previous PRs however I felt that squashing my carefully crafted commit sequence into one humongous commit lost something useful. I realize that squashing is important to get rid of noisy “fixup” commits and exploratory dead-ends that need no historical record. But that’s something I do on my end before even submitting a PR, and in my own projects I would also ask a contributor to do it on their end before I would consider a PR ready for merging (or even for reviewing). I am fine with deviating from my personal preferences though if there is a project policy. |
|
I think there are only personal opinions on that within this project. I personally object to merge commits that don't have a very good reason. I think "fixups" should absolutely be squashed, and when there is logical sequence of commits they should be rebased (and not squashed). As I do a lot of git history reading, I really appreciate logical commits with good quality summaries and messages. I previously objected to some PRs that were not squashed because they were fixups commits. Perhaps my objection was taken with too much weight and lead to overly squashed PRs. At the moment only rebased and squash are available in the UI IIRC. If there is a real need to add a merge on applying a pr, then the otterdog config needs to be temporarily updated. FWIW, on other projects, such as Eclipse Platform only rebase is allowed, and contributors are expected to do any needed squashing before final review and approval. |
|
My only "strong" opinion is about keeping a linear history for reproducible builds. And about not flooding the commit history with "prettier" fixes and the likes. 🙂 I personally prefer squash commits to achieve the above. But at the same time, I normally prefer to enforce an update of a branch before merge (which we don't have). So, a rebase would actually enforce that, too. Hence, I am not against changing the settings. I guess the only things to consider are if it raises the barrier for new contributors (probably not), and if we are in danger of losing efficiency on individual PRs by discussing how a clean commit history should look like (probably only a problem when we have more activity from different contributors with more or less strong opinions). If you think we should try different settings that still ensure a linear commit history then please go ahead and feel supported by me. |
|
It seems we are all in agreement about keeping a linear history, and as far as I am concerned the decision of whether to squash or not in a given case can be left to the good judgement of the reviewer. I see no need to change any settings of the GitHub UI, using that is not mandatory anyway (to my knowledge). Thanks for the guidance! |
3 participants
Is there a reason we don’t run
yarn lintduring PR CI? Let’s see if it works. Because if there is no reason, then I am going to add it, otherwise the code style tends to stray occasionally, as right now.Clean up that, as well as another typo from the same commit (#470).
Also, while I am messing around in this area, let’s run tests in a mode that catches accidentally committed
.only((see #415 (comment)).