Skip to content

Inline trivial API-wrapper actions#5149

Merged
chrisbobbe merged 3 commits intozulip:mainfrom
gnprice:pr-inline-trivial-api-actions
Dec 6, 2021
Merged

Inline trivial API-wrapper actions#5149
chrisbobbe merged 3 commits intozulip:mainfrom
gnprice:pr-inline-trivial-api-actions

Conversation

@gnprice
Copy link
Copy Markdown
Member

@gnprice gnprice commented Dec 3, 2021

This carries out a small cleanup I described at #4899 (review) (/cc @AkashDhiman):

1 more thing I did on my own in this revision is that I am using api.setSubscriptionProperty instead of dispatch(setSubscriptionProperty) used previously.
is there any difference among them?

Good question. As you can see in the implementation of the setSubscriptionProperty thunk action, it's a pretty trivial wrapper around api.setSubscriptionProperty -- it's not really adding any new behavior.

I think the answer is that that wrapper (and the similar trivial thunk-action wrappers we have for some other API calls) shouldn't exist, and we should just use the api.foo methods directly, just as you do in this revision.

That's because I don't think there's any meaningful semantics being added by those wrappers. That is, not only do they not add any significant behavior or logic, but there also isn't any way their semantics meaningfully differs from the underlying api.foo methods -- it's not like there's something they're abstracting away, that just happens to have a trivial implementation but where the fact the implementation is trivial is itself a nonobvious fact. (For some examples of functions with trivial implementations that do nevertheless have nontrivial semantics, see src/recipient.js -- particularly streamNameOfStreamMessage or pmTypingKeyFromPmKeyIds.) So they don't gain us anything.

And conversely, the extra wrapper has a real cost -- an extra layer of indirection to jump through when you're reading the code and working out what it does.

@gnprice gnprice requested a review from chrisbobbe December 3, 2021 03:23
This is just a wrapper for api.setSubscriptionProperty, and one that
doesn't add any meaningful semantics.  That is, not only does it not
add any significant behavior or logic, but there also isn't any way
its semantics meaningfully differs from the underlying function.

(Sometimes a function with a trivial implementation is nevertheless
abstracting something away, so that the fact the implementation is
trivial is itself a nonobvious fact.  But this isn't one of those.)

So, just inline it at its call sites.  We already have a series of
other sites that call `api.setSubscriptionProperty` directly.

Originally noticed this here:
  zulip#4899 (review)
Much like the preceding two commits.

These two aren't quite as vacuous as those before: they do adapt the
interface slightly, by providing two different functions in place of
one function that takes a parameters object.

Still, I don't think the interface they provide is much clearer than
the one from the underlying API; better to just have one fewer
indirection to trace through.
@chrisbobbe chrisbobbe force-pushed the pr-inline-trivial-api-actions branch from 6772d2d to c6ca8ad Compare December 6, 2021 21:43
@chrisbobbe chrisbobbe merged commit c6ca8ad into zulip:main Dec 6, 2021
@chrisbobbe
Copy link
Copy Markdown
Contributor

Thanks, LGTM! Merged.

@gnprice gnprice deleted the pr-inline-trivial-api-actions branch December 6, 2021 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants