-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Align parameters syntax with attributes #158
Conversation
@@ -766,30 +766,9 @@ Defined by the `Parameters` keyword written in a Markdown list item: | |||
#### Description | |||
Discussion of URI parameters in the _scope of the parent section_. | |||
|
|||
This section **must** be composed of nested list items only. This section **must not** contain any other elements. One list item per URI parameter. The nested list items subsections inherit from the [Named section](#def-named-section) and are subject to additional formatting as follows: | |||
This section **must** be composed of nested list items only. This section **must not** contain any other elements. Each of the list item describes a single URI parameter using the **[Markdown Syntax for Object Notation][MSON] (MSON)**. |
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.
Should this read: "Each of the list items" instead of "Each of the list item".
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.
👍 on @kylef's comment
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.
Also I am not convinced that we should refer to MSON here. Align the syntax yes. But I would not claim it is MSON syntax as there are some gaps such as the required vs. optional or the fact that there are no nested types.
Also explaining URI syntax here without references to MSON would set a great building stone for introducing MSON later...
``` | ||
|
||
You can give an enumeration of possible values using [MSON][] as shown below: |
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.
Please do not refer to MSON here. Explain it without referencing to MSON syntax.
@pksunkara reviewed. Besides @kylef comments I would say we can't really claim the URI parameter syntax IS MSON syntax, because, well, it isn't. Please take the effort and explain the syntax in words without referring to MSON. |
I strongly disagree with that. In my opinion, we should want to move URI parameter syntax to MSON syntax (not simply align it but complete move it) because of three things:
|
@pksunkara : I'm OK with adding fully-MSON parameter descriptions, but if your "move" means "forbid the old way" I would strongly disagree. We are using Blueprint to document legacy, often highly un-RESTful APIs (as well as documenting and developing more RESTful ones). If the Blueprint language should lose the power to do that, we couldn't continue using it. But I agree with @zdne in that the MSON syntax for parameters must be in place first, before it can be used by reference to explain things. |
About "move": OK, I'm satisfied about that. About "agree with @zdne":
I take Z's comment to mean "at the moment, URI parameter syntax is not MSON. Therefore, at the moment, we shouldn't put in a link that claims that it is MSON." Z's comment doesn't explicitly say whether the fix should be to remove the link (which would, of course, be easy), or to fix up URI parameter syntax to be MSON (as you have advocated, a more difficult but possibly higher goal). I took your "strong disagreement" to mean "it's OK to put the MSON link here, even though URI parameter syntax is not MSON." Perhaps you meant something lesser, like "this particular detail is already MSON-compatible, so the link is OK in this case." I would disagree with that, for fear of creating the false impression that URI parameter syntax is entirely MSON. Or, perhaps you meant "I agree that URI parameter syntax is presently not MSON, but I believe we should respond by fixing URI parameter syntax actually to be MSON, at which point this link would be OK." Now that you've reassured me about "move" and backwards compatibility, I'm OK with this philosophical position, but I continue to hold (in agreement, I believe, with Z) that the upgrades to URI parameter syntax should come first, and only when that is complete does this link become OK. And as background, I'm a little dubious that this upgrade of URI parameter syntax to full MSON-ness is actually practical, so I'd rather not see the link sneak in on mere speculation that MSON-ization will eventually happen. |
Okay, got it. Thanks for the exaplanation 😄 Firstly, the changes in this PR will get published only after they are implemented in the parser. So
This is what I am trying to do. I think what @zdne was trying to say is that, "Parameters should not be MSON, just the syntax should resemble a bit of MSON". That statement is what I strongly disagree with. I think it should be completely MSON not just resemblance.
This should not be an issue due to how MSON was implemented in the parser. If we do decide on going the MSON route, there will be no half-measures when implementing the change in the parser. |
@jrep thanks for great feedback! I think we are aligned here. Although I have a question: Do you think we should keep the deprecated syntax in the specification (or anywhere else for that matter)? The motivation behind this question is to prevent confusion for readers of older blueprints. |
Let's do it step by step. First align. Then move it if it will make sense.
Aligning the syntax first won't prevent supporting this in a future
Again, the alignment won't prohibit us from doing this in future.
Let's not be lazy. Aligning this is actually much easier and safer then referencing MSON. There is way too many exception in MSON that you would have to take care (read document) them than defining the primitives (consider On Of, Include, Inheritance, Nested members). |
Two main points here:
|
76be4f7
to
4fa3a31
Compare
Updated. |
@@ -766,17 +766,19 @@ Defined by the `Parameters` keyword written in a Markdown list item: | |||
#### Description | |||
Discussion of URI parameters in the _scope of the parent section_. | |||
|
|||
This section **must** be composed of nested list items only. This section **must not** contain any other elements. One list item per URI parameter. The nested list items subsections inherit from the [Named section](#def-named-section) and are subject to additional formatting as follows: | |||
This section **must** be composed of nested list items only. This section **must not** contain any other elements. Each of the list items describes a single URI parameter. The nested list items subsections inherit from the [Named section](#def-named-section) and are subject to additional formatting as follows: |
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.
I think this could be a little simpler:
-Each of the list items describes a single URI parameter.
+Each list item describes a single URI parameter.
Looks good to me. Main comment is on explaining |
Updated. |
Align parameters syntax with attributes
Align parameters syntax with attributes
Align parameters syntax with attributes
Align parameters syntax with attributes
For @zdne to review & merge.
I had all this description about how the old syntax was changed to the new syntax and stuff. Then, I realized that it is not the way we do things in the API Blueprint specification. 😐
We only describe the current syntax, but still offer backward compatibility.