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

Feat/typescript v31 rc #130

Merged
merged 11 commits into from
Jun 7, 2024
Merged

Feat/typescript v31 rc #130

merged 11 commits into from
Jun 7, 2024

Conversation

Alessandro100
Copy link
Contributor

#122

This PR adds support for gbfs v3.1-rc to typescript language bindings

Changes:

  • v3.1-RC introduced to typescript + tests
  • auto generated files are no longer committed to the repo and will be generated when needed
    - files are generated on postinstall yarn install
  • moved script logic out of typescript folder into root script folder
  • updated cicd to include test check
  • added testfixtures for v3.1-rc

@Alessandro100 Alessandro100 self-assigned this Jun 4, 2024
@emmambd emmambd requested a review from richfab June 4, 2024 15:17
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.

LGTM. Thanks @Alessandro100!

I added a few suggestions.

It would be good that a dev from MobilityData reviews this PR 🙏

"url": "https://platform-services.tier-services.io/data-sharing/tier_tier_fingal/gbfs/3.1-RC",
"version": "3.1-RC"
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion to add area and country_code in the manifest.json test to cover MobilityData/gbfs#572.

Example was found in https://github.com/MobilityData/gbfs/blob/master/gbfs.md#manifestjson.

Suggested change
]
],
"area": {
"type": "MultiPolygon",
"coordinates": [
[
[
[
13.10821,
52.58563
],
[
13.29743,
52.67046
],
[
13.48451,
52.6855
],
[
13.77993,
52.43458
],
[
13.65355,
52.33048
],
[
13.08165,
52.38793
],
[
13.10821,
52.58563
]
]
]
]
},
"country_code": "DE"

Comment on lines 8 to 11
"system_id": "tier_fingal",
"versions": [
{
"url": "https://platform-services.tier-services.io/data-sharing/tier_tier_fingal/gbfs/2.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be more neutral to use random names and URLs for the test files instead of those from a real operator. Thoughts @emmambd?

Example was found in https://github.com/MobilityData/gbfs/blob/master/gbfs.md#manifestjson.

Suggested change
"system_id": "tier_fingal",
"versions": [
{
"url": "https://platform-services.tier-services.io/data-sharing/tier_tier_fingal/gbfs/2.1",
"system_id": "example_berlin",
"versions": [
{
"url": "https://berlin.example.com/gbfs/2/gbfs",

Copy link

Choose a reason for hiding this comment

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

@richfab Makes sense to me!

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea

@richfab richfab requested a review from davidgamez June 5, 2024 08:23
@Alessandro100 Alessandro100 merged commit 09aade5 into master Jun 7, 2024
4 checks passed
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