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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Persist and serve user preferences #1184
Persist and serve user preferences #1184
Changes from 14 commits
cdd5178
260a356
22703b8
a5370e7
d04154c
ed520ba
32670e6
907d1e4
7d1ab56
e9c0e9c
4150a96
e514b52
ed4b92d
75478dd
87b39f2
eaabd1c
2a8371d
4f6f7ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I was wondering if you wanted to trim the
propertyName
. I noticed i'm able to make properties named ' ' and ' '.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.
🤔 No, in the end I think that would be bad. I don't think I should treat the property name as anything else but an opaque token, as it's just an identifier. Spaces in the URL path component for the property name (encoded though of course) are valid.
Tangentially, Github markdown squished (trimmed) your
' '
and' '
whitespace property name examples. That's sort of illustrative: it's because Markdown is for presentation, and it is documented to do that. Yet for the property names in the DB and API I don't see a benefit in constraining the property name (beyond refusing a zero-length string). They're chosen by developers, and of course a developer can choose a bad property name (including' '
) just like they can choose bad variable names, but presumably we'd catch that in review, just like we catch bad variable names.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 there another existing Problem we could use instead? I see this only comes up if you pass
propertyValue
AND extra stuff like{'propertyValue': 'meow', 'cats': ' '}
. Maybe we just acceptpropertyValue
and ignore the rest.I'm also ok leaving it as is with this new Problem because there is a test for it :)
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.
I did look at the existing Problems, and they had their problems. There's
unexpectedAttributes
but its comments and problem string then proceed to talk about arguments rather than attributes. I didn't feel like fixing that because due to the ambiguity some use sites will use it "because it's about arguments" and some others will use it "because it's about attributes" so that would require unraveling the original intent at each use site.I could probably have lived with using the existing
unexpectedAttributes
error even though its name implies we're talking about "attributes" rather than "properties" ("properties" is what we're dealing with in json), I'm not that zealous, but I drew the line at "arguments" as inunexpectedAttributes
's error text. Because talking about "arguments" will make for a confusing error message in the context of posting a JSON body, and I value a precise and appropriate error message more than I value saving a line of code in this case ;-)