-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
Doesn't really do much 🤷 |
Towards rocicorp#290
@@ -255,6 +257,7 @@ pub struct PullRequest { | |||
pub cookie: String, | |||
#[serde(rename = "lastMutationID")] | |||
pub last_mutation_id: u64, | |||
#[serde(rename = "pullVersion")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency but we should rename all of them to snake_case. I added an entry to the task list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really followed? Seems icky at least for js consumers because then the resulting js objects conflict with js style.
I personally prefer it as is but if you can show me that everyone else is using snake case I’ll go along.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JS API is still all camelCase. This is just for the json.
https://developer.mozilla.org/en-US/docs/Web/Manifest
https://developer.chrome.com/docs/extensions/mv3/manifest/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong preference for narrow gully hungarian notation, which would make this version integer VIpULLvERSION
TBR |
On Sat, Feb 27, 2021 at 12:43 PM Erik Arvidsson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/sync/pull.rs
<#311 (comment)>:
> @@ -255,6 +257,7 @@ pub struct PullRequest {
pub cookie: String,
#[serde(rename = "lastMutationID")]
pub last_mutation_id: u64,
+ #[serde(rename = "pullVersion")]
The JS API is still all camelCase. This is just for the json.
https://developer.mozilla.org/en-US/docs/Web/Manifest
https://developer.chrome.com/docs/extensions/mv3/manifest/
What is the distinction between the JS API and the JSON?
In Next.js, I consume the JSON and use it as JS objects via JSON.parse.
There's not really a decode step that I have fine control over...
The links you provided are for web platform APIs, and I don't feel like
it's really the same. I'm wondering if there's a strong convention to
prefer snake case for web service APIs.
—
… You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#311 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBGLKZMV7TS2OSIBJCTTBFYQDANCNFSM4YJJIGBQ>
.
|
On Sat, Feb 27, 2021 at 12:58 PM Aaron Boodman <aaron@aaronboodman.com>
wrote:
On Sat, Feb 27, 2021 at 12:43 PM Erik Arvidsson ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/sync/pull.rs
> <#311 (comment)>:
>
> > @@ -255,6 +257,7 @@ pub struct PullRequest {
> pub cookie: String,
> #[serde(rename = "lastMutationID")]
> pub last_mutation_id: u64,
> + #[serde(rename = "pullVersion")]
>
> The JS API is still all camelCase. This is just for the json.
>
> https://developer.mozilla.org/en-US/docs/Web/Manifest
> https://developer.chrome.com/docs/extensions/mv3/manifest/
>
What is the distinction between the JS API and the JSON?
In Next.js, I consume the JSON and use it as JS objects via JSON.parse.
There's not really a decode step that I have fine control over...
The links you provided are for web platform APIs, and I don't feel like
it's really the same. I'm wondering if there's a strong convention to
prefer snake case for web service APIs.
I see what you mean. You're saying the client side API is still camel case.
I get that -- it's only the server developer who sees these names.
It looks like there's no strong convention. The google styleguide says
camelcase:
https://google.github.io/styleguide/jsoncstyleguide.xml?showone=Property_Name_Format#Property_Name_Format,
but google maps uses snake :). Facebook and Twitter APIs both use snakecase.
I guess since I'm currently in JS land on the server constructing these
responses, so that's where the slight preference that it feels js-y is
coming from.
Guess I don't have that strong a preference is you feel strongly about
this. I can see the argument your way.
… —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#311 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAATUBGLKZMV7TS2OSIBJCTTBFYQDANCNFSM4YJJIGBQ>
> .
>
|
As long as it is consistent I do not care too much either. I was just trying to match some kind of industry convention but it seems like there really isn't enough of one. Using camelCase is nicer in some way because we do not need eslint comments everywhere. I'm just going to stick with what we have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -16,6 +16,8 @@ use std::collections::hash_map::HashMap; | |||
use std::default::Default; | |||
use std::fmt::Debug; | |||
|
|||
const PULL_VERSION: u32 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this next to the PullRequest definition, and maybe move the comment describing version 0 to it?
@@ -255,6 +257,7 @@ pub struct PullRequest { | |||
pub cookie: String, | |||
#[serde(rename = "lastMutationID")] | |||
pub last_mutation_id: u64, | |||
#[serde(rename = "pullVersion")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong preference for narrow gully hungarian notation, which would make this version integer VIpULLvERSION
Follow up to rocicorp#311
Towards #290