-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Change guidelines to forbid defaulting values #52094
Merged
neil-marcellini
merged 6 commits into
Expensify:main
from
callstack-internal:feature/no-default-values-guidelines
Nov 19, 2024
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ab94ee9
Change guidelines to forbid defaulting values
fabioh8010 088f1c3
Address comments
fabioh8010 208b743
Add explanation for default IDs
fabioh8010 d1e7405
Merge branch 'main' into feature/no-default-values-guidelines
fabioh8010 7bcabe0
Remove DEFAULT_STRING_ID and add more instructions
fabioh8010 23ccd7e
Adjust guidelines
fabioh8010 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
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'm confused by that suggestion.
Can we please include some examples for why we have these guidelines and our reasoning for them? I think explaining the why will help people follow the suggestions.
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 guess @iwiznia you meant
an ID
instead ofand ID
.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.
Same here as my previous comment. We all agreed in the proposal that we shouldn't default string IDs to any value, because they can be passed to Onyx when accessing records of a collection and cause bugs/crashes. And we also discussed here that in a exceptional case, if we really need to default the string ID to some value we are going to default them to
''
.Do you still agree with the proposed solution?
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.
Sounds good. Please also include an explanation for why we make these recommendations.
Something like "The default number ID is falsy so that checks like
!someID
work, and it's currently set to0
which is the same default value we use on the backend. We use a constant so it can be easily changed across the codebase if needed.". Please explain similarly for our guidance on number IDs.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.
Please also explain and give an example of when this is necessary
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.
@neil-marcellini I've added some explanations as requested, please let me know if it's good enough
@iwiznia Unfortunately I couldn't think of any examples defaulting the string ID would be necessary. We just added because, given the codebase's size, it could happen so we are already covering this situation in the guidelines. If you think this is not valid we can instead remove this exception and
DEFAULT_STRING_ID
for now.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.
Didn't we say in the thread you linked that we needed to default to
undefined
or0
when connecting to an onyx key?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.
That's true, but that would be not providing a default, or using a number right?
It's sounding like we really can't come up with a good case for defaulting a string. I think it would be more clear to leave that out entirely. We could add a note that if you find a reasonable exception or are confused about a case to raise your question in #expensify-open-source.
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 removed
DEFAULT_STRING_ID
and added the note.