-
Notifications
You must be signed in to change notification settings - Fork 25
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
"Type" missing in rates/getAll documentation.md #470
base: master
Are you sure you want to change the base?
Conversation
For rates/getAll, the response includes the type of rate (e.g. private, public, availability block). This is not mentioned in the documentation but in swagger. This value is very helpful to determine which rates are availability block rates and which ones are not.
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.
Hi @BrittaBecker, I have edited your patch directly, I thought that would be the easiest. Since this way of working is still new, I thought it would be helpful to describe what I changed in detail here:
- Added a changelog entry - I think this change is substantial enough that it should have an entry in the changelog.
- Edited
Type
so that it refers to a separate string enumeration entitledRate type
, and added that string enumeration; this is the normal way this would be handled in the docs. - Following the suggestion of @vitezp, I added the other missing property
Description
.
There is one thing still missing, however - we need to add examples of the two new fields to the JSON response example. Is this something you could do?
And finally, I would like someone from Dev to approve the changes before they are accepted into the main branch.
Thanks :-)
I've added the "type" parameters in the code example. Additionally, I noticed that the "options" parameter was also not described, so I added this as well. @MikeAdamsMews could you please check and adjust the "options" description if needed? Thanks!
"ExternalNames": {}, | ||
"Description": {}, | ||
"ExternalIdentifier": null, | ||
"Options": { |
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.
What is Options? This has sneaked in to the PR.
"Name": "Fully Flexible Rate", | ||
"ShortName": "FLEXFLEX", | ||
"ExternalNames": {}, | ||
"Description": {}, |
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.
We prefer to use real examples, rather than just blank. I guess it would be something like this:
"Description": {
"en-US": "Fully flexible rate",
"de-DE": "Völlig flexibler Tarif"
},
| `ExternalIdentifier` | string | optional, max 255 characters | Identifier of the rate from external system. | | ||
| `Options` |[Localized text](_objects.md#localized-text)| required | Rate options. | |
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.
As above, what is Options? I checked the YAML and it's not in there.
Hi @BrittaBecker, when I reviewed this PR in August I raised a couple of questions, could you take a look please. |
Hi @MikeAdamsMews, I've checked the points.
|
For rates/getAll, the response includes the type of rate (e.g. private, public, availability block). This is not mentioned in the documentation but in swagger. This value is very helpful to determine which rates are availability block rates and which ones are not.