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

Improve readability #576

Closed
wants to merge 4 commits into from
Closed

Conversation

tdelmas
Copy link
Contributor

@tdelmas tdelmas commented Dec 9, 2023

What problem does your proposal solve? Please begin with the relevant issue number. If there is no existing issue, please also describe alternative solutions you have considered.

This PR tries to improve the readability of some points:

  • Objects and array of objects, which are too often described as "An array of objects, each containing the keys below."
  • REQUIRED field inside OPTIONAL objects, which are marked "Conditionally REQUIRED" with the description "REQUIRED if (name of the object) is defined".

Those make the specification too much verbose, thus blurs the important things. For example, when a field is "Conditionally REQUIRED", it's important to clearly understand why, and having too many fields defined as such without a reason make the specification unnecessarily complicated

What is the proposal?

  • Describe the type object, to say once at the top that the keys are defined below.
  • Clarify that a REQUIRED field in an OPTIONAL object is only required when the object is present

Is this a breaking change?

  • Yes
  • No
  • Unsure

@tdelmas
Copy link
Contributor Author

tdelmas commented Dec 9, 2023

Ping @richfab : Can I have your opinion on those changes?

Copy link
Contributor

@richfab richfab left a comment

Choose a reason for hiding this comment

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

Thank you @tdelmas for this contribution! We all love precise and concise documentation 💯

I gave my personal opinion as requested. Curious to know what others think!

\- `brand_image_url_dark` <br/>*(added in v2.3)* | OPTIONAL | URL | A fully qualified URL pointing to the location of a graphic file representing the brand for the service for use in dark mode applications. File MUST be in SVG V1.1 format and MUST be either square or round.
\- `color` <br/>*(added in v2.3)* | OPTIONAL | String | Color used to represent the brand for the service expressed as a 6 digit hexadecimal color code in the form #000000.
`terms_url` <br/>*(as of v3.0-RC)* | OPTIONAL | Array&lt;Localized String&gt; | A fully qualified URL pointing to the terms of service (also often called "terms of use" or "terms and conditions") for the service.
`terms_last_updated` <br/>*(added in v2.3)* |Conditionally REQUIRED | Date | REQUIRED if `terms_url` is defined. The date that the terms of service provided at `terms_url` were last updated.
`terms_last_updated` <br/>*(added in v2.3)* | REQUIRED | Date | The date that the terms of service provided at `terms_url` were last updated.
Copy link
Contributor

@richfab richfab Dec 11, 2023

Choose a reason for hiding this comment

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

I like that the proposed solution is more compact and less verbose. I am slightly concerned that the proposed solution may be less obvious for the newcomers to the spec.

Curious to know what @josee-sabourin and others think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the opposite feeling : conditionals are hard to read, and having a required field in an optional object is, in my opinion, easier to understand, especially for newcomers. But I want to see if others are feeling something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging recent contributors to get your opinion: @mplsmitch, @testower, @cmonagle, @simonsolnes, @ezmckinn, @AntoineAugusti, @ArashMansouri, @jkurzanski.

What is your view on the proposal to make the specification less verbose for REQUIRED fields inside OPTIONAL objects?

Before After
Conditionally REQUIRED with the mention "REQUIRED if object is defined" REQUIRED

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @tdelmas. If the object itself is optional, there's no need to repeat that information on each property of the object.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the changes!

@josee-sabourin
Copy link
Contributor

josee-sabourin commented Dec 20, 2023

My two cents:
Personally, I prefer the more explicit version of Conditionally Required / Required if simply because I don't think the "-" before what is contained in an object, or the tab for subsequent objects within that object are super clear. See image below:
Screenshot 2023-12-20 at 12 59 42 PM

If we can make these dashes more explicit, then I would be in favour of this proposal.

@tdelmas
Copy link
Contributor Author

tdelmas commented Dec 20, 2023

If we can make these dashes more explicit, than I would be in favour of this proposal.

@josee-sabourin I 100% agree with that analysis. Any suggestion to make it clearer? Maybe one dash per level?

|----------------|---|---|
|`rental_apps`   |   |   |
| - `android`    |   |   |
| - - `store_uri`|   |   |

@josee-sabourin
Copy link
Contributor

A dash per level would be more clear. We could also amend the Object field definition to say "The key value pairs contained within an object are identified by a dash (-). In the instance of an Object within an Object, an extra dash (-) is present."?

The last sentence needs some more work. Open to suggestions.

@mplsmitch
Copy link
Collaborator

  1. the definition for object at line 191 should be combined with, or replace the existing object definition in the alphabetized list at line 225
  2. I'm all for improved readability but I agree with the 2 cents from @josee-sabourin. I think it's pretty easy to misinterpret the "-" and tabs currently used in the Field Name column. One dash per level is an improvement. Another option would be to use the format that Google uses in their GBFS definitions, for example :
    plans
    plans[].per_km_pricing
    plans[].per_km_pricing[].start

@richfab
Copy link
Contributor

richfab commented Dec 21, 2023

+1 for using the format that Google uses in their GBFS definitions.

@richfab richfab mentioned this pull request Jan 4, 2024
3 tasks
@richfab
Copy link
Contributor

richfab commented Jan 4, 2024

Closing in favor of #583

@richfab richfab closed this Jan 4, 2024
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.

6 participants