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

V3.0-RC Schemas #79

Merged
merged 3 commits into from
Apr 11, 2023
Merged

V3.0-RC Schemas #79

merged 3 commits into from
Apr 11, 2023

Conversation

josee-sabourin
Copy link
Contributor

New directory of schemas for v3.0-RC.

Specifically looking for input on system_information.license_url and license _id.
Used anyOf similarly to the way it's been used in vehicle_status with lat, lon and station_id in previous versions. Is that the best way of representing this?

josee-sabourin added 2 commits March 9, 2023 20:00
formatting
add anyOf to system_information for licensing
@isabelle-dr
Copy link
Contributor

isabelle-dr commented Mar 16, 2023

A few meta comments:

  • it's a little difficult to review everything in one commit, in the next one I think we could have one commit per PR merged as part of the release.
  • the structure of gbfs.json seems different from the previous version, is this on purpose? I used this tool to compare the gbfs.json files in the v2.3 and v3.0-RC folders. Some fields aren't at the same nesting level.

I will update this list as I am reviewing this PR!

@josee-sabourin
Copy link
Contributor Author

Noted on using multiple commits next time, that was my bad!

As for gbfs.json, yes this is correct. The PR for internationalization removes the need to duplicate your feed per language, so that nesting level is gone.

@isabelle-dr
Copy link
Contributor

Sounds good!
I've created a fake PR to be able to compare v2.3 and v3 using GitHub's diff interface.

v3.0-RC/gbfs.json Outdated Show resolved Hide resolved
}
}
},
"anyOf": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Here my understanding of the spec is that license_id and license_URL can both be left blank and that license_URL must not be provided if license_id is present.

I've tested:

  • license_id and license_URL are present -> invalid ✅
  • onlylicense_id is present -> valid ✅
  • onlylicense_url is present -> valid ✅
  • license_id and license_URL are not present- > invalid ❌ (we want this to be valid)

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we need something else than anyOf to validate this 🤔

Copy link
Contributor Author

@josee-sabourin josee-sabourin Mar 22, 2023

Choose a reason for hiding this comment

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

I think perhaps oneOf might be the proper solution and not required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asking for help from @testower or @AntoineAugusti (or anyone else!), oneOf doesn't seem to solve the problem, since it still requires one of the options, I can't seem to determine how to represent one of or none.

Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

From https://stackoverflow.com/a/38799750/2645887

Maybe

"dependencies": {
  "license_url": { "not": { "required": ["license_id"] } },
  "license_id": { "not": { "required": ["license_url"] } }
},

Copy link
Contributor Author

@josee-sabourin josee-sabourin Apr 6, 2023

Choose a reason for hiding this comment

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

This seems to work! Merci @AntoineAugusti!! @isabelle-dr do you want to confirm as well or should I make the change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@josee-sabourin I've just pushed a commit that fixes this problem, please confirm!
It does the following:

  • license_id and license_URL are present -> invalid
  • onlylicense_id is present -> valid
  • onlylicense_url is present -> valid
  • license_id and license_URL are not present- > invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

confirmed!

},
"opening_hours": {
"description": "Hours and dates of operation for the system in OSM opening_hours format. (added in v3.0-RC)",
"type": "string"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you look into using a Regex expression to validate the opening hour?

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, but I have not been able to find one that works for all possibilities.

@isabelle-dr
Copy link
Contributor

Okay, I am finished with this review! I've added two comments in-line.
Besides this, three meta comments:

  1. We should open an issue on the GBFS validator to ass a custom rule to validate the new conditional requirement added here for default_reserve_time in vehicle_types.json:

REQUIRED if reservation_price_per_min or reservation_price_flat_rate are defined.

  1. Do we want operators to tag their version as 3.0-RC, or 3.0? The JSON Schema is using 3.0-RC and the examples in the spec is using 3.0. It might be more clear to use the same in both places.

  2. We should remove Extending vehicle_types.json gbfs#370 from the list of PRs in v3.0-RC release, it was part of the V2.3

@josee-sabourin
Copy link
Contributor Author

Some answers for you:

  1. Will do!
  2. For now we want operators to use v3.0-RC, I will update the examples on the spec
  3. See comment here showing that 370 is actually a change for both 2.3 and 3.0

@josee-sabourin josee-sabourin added the help wanted Extra attention is needed label Apr 5, 2023
Copy link
Contributor

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

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

Ready to go!
Awesome work @josee-sabourin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants