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
feat(auth): Add enableAnonymousUser param to CreateTenant and UpdateTenant #412
feat(auth): Add enableAnonymousUser param to CreateTenant and UpdateTenant #412
Changes from 3 commits
ba6e78b
0f7899f
a3dfd67
2600d00
41db8d3
9d6bfe9
6e3fc83
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.
Looks like
EnableAnonymousUsers
is a literal that should be backticked for code font.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.
It doesn't look like we back-tick literals in Go SDK. ex:
TenantClient
,AndroidNotification
etc.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.
Our auth docs use the term "anonymous authentication." Should that be used here?
I'd suggest that phrase, or, alternatively, "anonymous user access."
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.
Looks like
EnableAnonymousUsers
is a literal that should be backticked for code font.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.
Our auth docs use the term "anonymous authentication." Should that be used here?
I'd suggest that phrase, or, alternatively, "anonymous user access."
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.
This makes sense, but in Go SDK we normally mention the function name followed by a description. Ex:
AllowPasswordSignUp enables or disables email sign-in provider.
orEnableEmailLinkSignIn enables or disables email link sign-in.
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.
Hm, OK, no backticks I could live with. But just "anonymous user" doesn't sound to me like a thing that can be enabled or disabled. Does it to you Lahiru?
I'd say that "email link sign-in" does sound like it can be disabled/enabled. FWIW at this point, "email sign-in provider" does not; "email sign-in provider support" or "email sign-in provider functionality" might.
This sounds like I'm splitting hairs, but one of the few things I bring to the table is an ear for written English. :) Let's get Kevin's input as well!
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.
Agreed.
"EnableAnonymousUsers enables or disables anonymous authentication."
(And FWIW, "AllowPasswordSignUp enables or disables the email sign-in provider.")
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.
+1 Yup, "the" solves that latter one!
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.
Oh I misread your previous comment, sorry! I understand what we are referring to now.
I am good with
// EnableAnonymousUsers enables or disables anonymous user authentication.
Thank you!