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
Who Added Me #614
Who Added Me #614
Changes from 20 commits
977176f
397096b
b7d9431
3257ef4
09cca4c
520ebe1
1519f2b
8ebeb1a
c8c54dd
292a24f
0bed242
76f08d0
9aa969d
a1f58c0
0f83abd
91d850d
c0871f8
ee1901c
3fc71ca
a795b0f
e75d5c0
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.
We should read this from
convo
so that the value is guaranteed to be consistent withconvo
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.
Good catch. Thank you!
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.
@zombieobject I think @richardhuaaa was suggesting that this is NOT NULL do you mind doing that in a follow up PR?
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.
@nplasterer I missed that comment just as I was merging the PR. I am more than happy to create a follow up PR to address this request.
For context, there were 2 reasons for the optionality of that parameter. The largest one IMHO was due to MLSGroup lifecycle, being that the address of "who added me" was not always available at group creation. The latter was breaking changes, but as you both stated, better to break things now, to which I concur.
I'll open up a follow up PR with the request immediately and see what I can do to address the lifecycle questions. 🙂