Skip to content
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

identity: require aliases to be anchored #1413

Merged
merged 1 commit into from Oct 19, 2019
Merged

identity: require aliases to be anchored #1413

merged 1 commit into from Oct 19, 2019

Conversation

@wchargin
Copy link
Member

wchargin commented Oct 18, 2019

Summary:
All the documentation and tests seem to be assuming that aliases must be
anchored: github/torvalds, but not some github/torvalds stuff.
JavaScript regular expressions aren’t anchored by default; this commit
adds explicit anchoring and adds tests.

Test Plan:
Unit tests added.

wchargin-branch: alias-anchor

@wchargin wchargin requested a review from decentralion Oct 18, 2019
@wchargin wchargin force-pushed the wchargin-alias-anchor branch from 48cf776 to 872e024 Oct 18, 2019
@@ -17,25 +17,33 @@ import {userAddress as discourseAddress} from "../discourse/address";
*/
export type Alias = string;

const _VALID_ALIAS = /^(\w+)\/@?(.*)$/;

This comment has been minimized.

Copy link
@Beanow

Beanow Oct 18, 2019

Member

Although this will be covered by the regex for each username. Why not use (.+) for the username part?

This comment has been minimized.

Copy link
@wchargin

wchargin Oct 19, 2019

Author Member

Type-specific parsers should handle that. Maybe we’ll add an self/
alias that always refers to the main project node or something, and has
no need for a username. The .* pattern is simpler than .+, so we
should prefer it unless there’s a reason not to.

This comment has been minimized.

Copy link
@Beanow

Beanow Oct 19, 2019

Member

Yeah that was what I was thinking. To have a bit of bias over what the username specific form can look like (similar to stripping @'s). But so long as we're exposing the function and not the pattern, either works for me :]

This comment has been minimized.

Copy link
@wchargin

wchargin Oct 19, 2019

Author Member

Yep, agreed. You’re right that this is a bit inconsistent; we should
probably push the @-stripping down, too. Done.

@Beanow
Beanow approved these changes Oct 19, 2019
@wchargin wchargin force-pushed the wchargin-alias-anchor branch from 872e024 to 4805b95 Oct 19, 2019
Summary:
All the documentation and tests seem to be assuming that aliases must be
anchored: `github/torvalds`, but not `some github/torvalds stuff`.
JavaScript regular expressions aren’t anchored by default; this commit
adds explicit anchoring and adds tests.

Test Plan:
Unit tests added.

wchargin-branch: alias-anchor
@wchargin wchargin force-pushed the wchargin-alias-anchor branch from 4805b95 to 5d2f057 Oct 19, 2019
@wchargin wchargin merged commit 28b25c2 into master Oct 19, 2019
2 checks passed
2 checks passed
ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@wchargin wchargin deleted the wchargin-alias-anchor branch Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.