-
Notifications
You must be signed in to change notification settings - Fork 162
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
editorial: shortcut compares wrong scope #886
Conversation
Will need to merge #882 first... as this depends on that. |
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 is a tricky PR to review ... it's making two separate changes (one to the definition of the "within scope" algorithm, which should have no behavioural changes, and one to fix the bug in shortcuts), as well as pulling in the name change to the within scope algorithm from the other PR, and changing a bunch of HTML syntax to Respec.
Could you separate some of these out? I suggest only fixing the shortcut bug here, and delaying the other changes to the scope algorithm until we've sorted out the naming on the other PR.
<ol class="algorithm"> | ||
<li>If |target| and |scope| are not [=same origin=], return `false`. | ||
</li> | ||
<li>Let |scopePath:string| be the [=string/concatenation=] of |
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.
"using" is correct here. See the definition of concatenation. This is supposed to place "/" in between each piece of the path, not at the end of it.
Perhaps you could clarify it by writing "using the separator "/"" or "using "/" as a separator".
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.
Ok, I thought that was weird.... was going to ask about that.
<li>If <var>target</var> is <a>same origin</a> as <var>scope</var> and | ||
<var>targetPath</var> starts with <var>scopePath</var>, return | ||
<code>true</code>. | ||
<li>If |targetPath| starts with |scopePath:string|, return `true`. |
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.
Maybe this and the next line could be combined by saying "Return a Boolean indicating whether |targetPath| starts with |scopePath|." Or even just "Return true if |targetPath| starts with |scopePath|, or false otherwise."
Since if I were writing this in C++, I would write:
return targetPath.starts_with(scopePath);
not
if (targetPath.starts_with(scopePath))
return true;
else
return false;
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.
Agree... much nicer.
index.html
Outdated
"within-scope-manifest">within scope of a manifest</dfn> | ||
<var>manifest</var> if <var>target</var> is <a>within scope</a> of the | ||
navigation scope of <var>manifest</var>. | ||
A [=URL=] |target:URL| is said to be <dfn data-export="" data-local-lt= |
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 seems to have stuff leaking into it from the other PR? (I hope you don't change this definition in the same PR as making the other changes.)
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.
The two PRs seem very intertwined. Maybe hold off on this one until that one is resolved.
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.
yeah... sorry, "Will need to merge #882 first... as this depends on that."
Gahh, I see, all of the comments I put here were actually for changes on #882. Will move them. |
Will send again once #882 is merged... |
Closes #885
Depends on #882 ...
I'm going to say this is editorial, as it was clearly not what was was intended...
This change (choose one):
changes normative sections without changing behavior)
Implementation commitment (delete if not making normative changes):
Commit message:
Fixes shortcut being compared to incorrect scope.
Preview | Diff