add isFeedType, isMsgType, isBlobType

[mixmix]

generalise some of the type checkers such that e.g.

• isFeed behaves the same
• isFeedType checks for things which follow the general pattern for a string encoded feed

[mixmix]

published

[staltz]

isFeedType(string), isMsgType (string), isBlobType(string)

Hey @mixmix, this is a decent addition and I don't have any objections to it. It's great you have @chereseeriepa in the same locality and can get timely code reviews.

This PR is fine, nothing needs to be done about it, but I have some feedback on how to structure a PR in general, as we are cooperating on PRs this week it seems. I'm using this one just as an example because it's an easier one to comment on.

Small PRs are generally the best to review, because they are easier to review, nothing is out of context, it's easier to predict how the new code will behave, it's easier to see in the future with a git blame why something was changed. I noticed you tend to make rather large PRs, or at least you bundle a couple of extra decisions into the same PR or even the same commit.

For instance in this case, the PR title is add isFeedType, isMsgType, is BlobType. That sets up the expectation that this is the only change this the PR does, but in reality it also did some shuffling around and refactoring of isMultiServerAddress and other things. In this case it's easy to see that it's unrelated to the addition of the new APIs, but it's not always easy to spot an extra refactor and separate it from the primary changes.

So I would recommend splitting these changes, at least put them in different commits, so the reviewer can click on a commit and look at only one "context" at a time. The ideal scenario is 1 commit per PR (unless you're doing 1st test, 2nd fix), so sometimes I "work on several aspects at once" and then I split those several changes into many commits, and for each commit I make a new branch that'll become a PR. For example, instead of this:

| * a844dcf4 - (9 weeks ago) refactor that thing — staltz (my-pr)
| * 6797d6bd - (9 weeks ago) refactor this thing — staltz
| * f788f80f - (10 weeks ago) add a feature — staltz
|/  
* cde4e2f6 - (4 days ago) release 1.2.0 — staltz (main, origin/main)

I would do this:

| * a844dcf4 - (9 weeks ago) refactor that thing — staltz (my-third-pr)
| * 6797d6bd - (9 weeks ago) refactor this thing — staltz (my-second-pr)
| * f788f80f - (10 weeks ago) add a feature — staltz (my-first-pr)
|/  
* cde4e2f6 - (4 days ago) release 1.2.0 — staltz (main, origin/main)

And then I submit one PR at a time, in order. When the 1st is merged, I submit the 2nd, and so forth. This is great, because if one PR is rejected, it doesn't block the other PRs. For instance, if you really need a feature shipped, just make a small PR for that feature. If it would be a big PR containing many changes, you could get stuck on back and forth code reviews on an unrelated refactor that just happens to be attached to the feature PR. So small PRs reduce disappointment too, because the stakes are lower with each PR.

How to know that a PR is "sufficiently small"? In my opinion it's not about actual diff size, it's more about how many "change decisions" a PR contains. For instance the decision to add standard may create a lot of diffs, but it's just one decision. Here's an example PR: https://github.com/ssb-ngi-pointer/ssb-meta-feeds/pull/9 I added "prettier" and nothing else, so the reviewer knows for sure that all diffs they are looking at are caused by the application of only one decision.

I hope this is useful feedback :pray:

[mixmix]

Yep, I am in total agreement with you @staltz I would atu this is the role I follow too, and also that I treat that rule as a guide.

In the case of this PR I see what you mean about splitting it into commits making it easier.

I also consciously did not change any logic, I just grouped and labeled things clearly. There's a balance for me between the cognitive load of splitting things into different PRs nicely. With this PR, I was mildly annoyed to be over here adding this (I just wanted to merge some ssb-bfe changes so I can work on the primary goal of subgroups).

This felt like a yak I didn't really want to be shaving. But I don't want to leave the yak messy, so I left the yak tidier than when I found it. If you asked me to do more work, I would probably just leave the yak messy.

Maybe that would have been fine. Maybe I need to be less worried about messy yaks

I feel like we're aligned on everything but when to bend our rules. I'll definitely add a filter to try and split off yak tidying into different commits.