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

Add additional fields for parking lots #216

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

leonardehrenfried
Copy link
Contributor

In #175 we discussed adding more fields to the parking lot schema.

This is my first attempt to do this but I acknowledge that this doesn't completely match with the list @kiliankoe provided.

This PR is just the first attempt and I'm happy to rework it as needed. Lets discuss!

@leonardehrenfried
Copy link
Contributor Author

One obvious problem is that the lot_type would have to be changed for all of the current parsers and perhaps the clients use the current values.

@jklmnn
Copy link
Member

jklmnn commented Jul 8, 2020

I like the new lot_type but maybe we can change it to lot_type_v2 or something similar since I'd like to keep backwards compatibility, especially since this API will definitely have a successor (whenever this will happen).

@leonardehrenfried
Copy link
Contributor Author

Would it not make sense to have a separate endpoint for v2 of the API? This would of course mean a bit of refactoring.

@leonardehrenfried
Copy link
Contributor Author

leonardehrenfried commented Jul 8, 2020

This would however allow us to change the times to use proper ISO 8601 with an offset (2020-07-08T15:21:08+02:00) rather than the current timezoneless type which implies UTC.

@jklmnn
Copy link
Member

jklmnn commented Jul 8, 2020

Would it not make sense to have a separate endpoint for v2 of the API? This would of course mean a bit of refactoring.

Yes this makes sense and there's already #140 to address this issue.

The documentation you want to change however addresses the current version (but it isn't up to date tbh). I don't want to mix the outdated version 1 documentation with features for a (yet undefined) version 2 API. Calling it 1.1 would also imply backwards compatibility.

@kiliankoe
Copy link
Member

kiliankoe commented Jul 8, 2020

I am very much against adding something like lot_type_v2 and I also don't see any merit in providing backwards compatibility. The old format is not well designed and it makes much more sense to apply any lessons learned and add/rename any fields that make sense for a new spec.

Let's create a new spec instead of modifying the old (and as @jklmnn stated outdated) one. Maybe even move that into its own repo to version it apart from the crawler/API?

@hbruch
Copy link
Contributor

hbruch commented Jul 28, 2020

I agree that we should not add a lot_type_v2. On the other hand, we'd like to add the new attributes url, opening_hours, fee_hours rather soon, while a new v2 api would require a bit of thinking, discussion, and work.

I'd suggest to remove lot_type from this PR and add the backward compatible new attributes url, opening_hours, fee_hours. WDYT?

@jklmnn
Copy link
Member

jklmnn commented Jul 28, 2020

I'd suggest to remove lot_type from this PR and add the backward compatible new attributes url, opening_hours, fee_hours. WDYT?

This would work for me.

@leonardehrenfried leonardehrenfried force-pushed the add-fields branch 2 times, most recently from 5bbb925 to 2a1a1bb Compare July 29, 2020 08:11
@leonardehrenfried
Copy link
Contributor Author

I have amended this PR to remove the field lot_type from the schema. It will remain unspecified.

However I'm seeing a problem on Travis, which appears to be happening when the Python requirements are installed: https://travis-ci.com/github/mfdz/ParkAPI/jobs/366048314

Do you have an idea what the issue is?

@jklmnn
Copy link
Member

jklmnn commented Jul 30, 2020

Do you have an idea what the issue is?

This is going to be fixed in #217. Please rebase your branch onto the new master once it is merged.

@leonardehrenfried
Copy link
Contributor Author

leonardehrenfried commented Jul 31, 2020

I've rebased and CI is green. Thanks!

@jklmnn jklmnn requested a review from kiliankoe July 31, 2020 12:09
Copy link
Member

@kiliankoe kiliankoe left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

4 participants