api: Document, and sweep for, our practice of making constructor arguments explicit #367
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…icit This came up in a code review here: zulip#361 (comment) and it'd be good to have written down in a more accessible place. There are some existing parts of the API bindings -- particularly the parts written in the very first week or two of the prototype, before I thought through this pattern -- that don't follow this. I'll fix those up next.
This appears to have been done in commit zulip/zulip@bd2545b0d. It's fine for it to have gone away -- the new API is better, and we weren't using this field -- but there's no notice of the change in the API changelog or the endpoint's own API docs. That shouldn't happen. I'll follow up. Anyway, now that I've sleuthed out the mystery of what happened, remove this from our bindings to match.
This reflects the real semantics, and will simplify things if we ever actually use this field. Also clarify what the todo-server comment refers to.
This completes the sweep for the point of style we added to the README a few commits ago.
|
Thanks, LGTM! Merged. |
Followed up on this in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This came up in a code review at #361 (comment) , and it'd be good to have written down in a more accessible place. Add this to the README (which for now doubles as our proto-style-guide):
Then there are some existing parts of the API bindings — particularly the parts written in the very first week or two of the prototype, before I thought through this pattern — that don't follow this. So sweep through fixing those, and leave explanatory comments on the only two places left as exceptions to the pattern.