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

Add uri sample with query showing it isn't captured #960

Merged

Conversation

JordonPhillips
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@JordonPhillips JordonPhillips requested a review from a team as a code owner October 25, 2021 13:11
@david-perez
Copy link
Contributor

I would explicitly call out somewhere that labels and greedy labels can only capture path segments from the URI. A far as I can see there is no mention in the spec as to what exactly do labels capture from the URI.

In the example(s), I would add #fragment to the end of the URI to convey that path fragments are also not captured by labels, or provide a separate example. I would also add the example(s) to the "Greedy labels" section.

@JordonPhillips JordonPhillips force-pushed the path-and-query-are-separate branch from ced0428 to 82a7839 Compare October 25, 2021 13:51
Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good but what I'm missing is a sentence in the spec saying something like "Labels can only capture path segments".

As of now, I think the spec is ambiguous as to what exactly do labels capture. Given:

  1. Patterns MAY contain label placeholders in the path,
  2. Labels MUST NOT appear in the query string; and
  3. Labels MUST NOT appear in the fragment,

then the pattern /foo/{bar+} adheres to the above three rules, but given the URI path /foo/quux?key=value#fragment, the spec doesn't explicitly say that the label {bar+} will only capture the string quux and not the string quux?key=value#fragment. With these changes, the examples implicitly convey this, but the question "what exactly do labels capture?" is never directly addressed by the spec.

@JordonPhillips
Copy link
Contributor Author

what I'm missing is a sentence in the spec saying something like "Labels can only capture path segments".

I added that in already:

Labels only capture path segments.

@david-perez
Copy link
Contributor

Ah, missed that in the diff. Looks good.

Commit message needs rewording before pushing, since now we're making the spec clearer in this respect and we're adding two examples, one for query string, one for fragment.

This adds some clarification about what precisely uri labels capture,
mostly through adding several more examples.
@JordonPhillips JordonPhillips force-pushed the path-and-query-are-separate branch from 82a7839 to 757eb4c Compare October 25, 2021 17:16
@JordonPhillips JordonPhillips merged commit 77e630d into smithy-lang:main Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants