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

modify data type of contribution type #135

Merged
merged 3 commits into from
Mar 4, 2021
Merged

Conversation

FabiKo117
Copy link
Contributor

@FabiKo117 FabiKo117 commented Feb 18, 2021

Description

use correct boolean instead of "true" as string

Checklist

@tyrasd tyrasd added the breaking This will introduce an incombability to previous versions. Documentation update compulsory label Feb 18, 2021
@tyrasd tyrasd added this to the 2.0 milestone Feb 18, 2021
@tyrasd
Copy link
Member

tyrasd commented Feb 18, 2021

strictly speaking this would be a major breaking change, and would need to wait for the 2.0.0 release (api version v2). Or do we think that this is actually a bug which needs to be fixed in the current v1?

@tyrasd
Copy link
Member

tyrasd commented Feb 18, 2021

slightly related:

I think we also misuse the properties=metadata parameter to trigger the output of the contribution type flags. Also the properties parameter is currently not documented for the contribution extraction endpoint(s). IMHO the following would have made more sense:

properties – specifies what properties should be included for each feature representing an OSM contribution (i.e. edit to an OSM element): tags and/or metadata and/or contributionType; multiple values can be delimited by commas; default: empty

@FabiKo117
Copy link
Contributor Author

strictly speaking this would be a major breaking change, and would need to wait for the 2.0.0 release (api version v2). Or do we think that this is actually a bug which needs to be fixed in the current v1?

Good question.. I am okey with either options. I also think that after 1.4, it could be time to go for the v2 release as we could also include the advanced response structure then into production. Then also this change/fix here could go live.

Otherwise there are people like @Zia- that would use it immediately and you could also see this as a fix, therefore it could be included in the next patch or minor release too.

@tyrasd
Copy link
Member

tyrasd commented Feb 18, 2021

I'm actually also a bit torn. Could live with both options for the "true" -> true fix. Not sure about the proposed #135 (comment) though.

@FabiKo117
Copy link
Contributor Author

FabiKo117 commented Feb 18, 2021

slightly related:

I think we also misuse the properties=metadata parameter to trigger the output of the contribution type flags. Also the properties parameter is currently not documented for the contribution extraction endpoint(s). IMHO the following would have made more sense:

properties – specifies what properties should be included for each feature representing an OSM contribution (i.e. edit to an OSM element): tags and/or metadata and/or contributionType; multiple values can be delimited by commas; default: empty

True, we're not mentioning the use of properties and clipGeometry in the docs. Only the geometryType is mentioned. Do you think that it would be necessary to make the contributionType another value of the properties parameter?

@tyrasd
Copy link
Member

tyrasd commented Feb 18, 2021

clipGeometry

isn't that one undocumented for a reason? 😉

@bonaparten
Copy link
Contributor

In relation to version 2.0, I think we should discuss soon how we want to proceed after the 1.4 release since we have at that moment already two PRs "blocked" because of breaking changes these PRs would introduce.

@tyrasd
Copy link
Member

tyrasd commented Feb 18, 2021

Do you think that it would be necessary to make the contributionType another value of the properties parameter?

I would prefer it that way. Because the contribution type has not much to do with the OSM elements' metadata (@version, @changesetId, etc.)

@FabiKo117
Copy link
Contributor Author

clipGeometry

isn't that one undocumented for a reason? 😉

It is mentioned here, what I mean is that it's not mentioned in the docs part of the /contributions endpoint. Also the properties parameter is not mentioned there, but can be used.

@tyrasd tyrasd added the bug Something isn't working label Feb 24, 2021
@FabiKo117
Copy link
Contributor Author

FabiKo117 commented Feb 24, 2021

So did I understand it correctly now out of the recent discussions that we define this as a bug-fix and can therefore include it in the next minor release?

The thing regarding the new property contributionType: This could be handled in another issue/PR I'd say and would just be a new feature and non-breaking (as long as it's still included when metadata is set as well).

@tyrasd
Copy link
Member

tyrasd commented Feb 24, 2021

So did I understand it correctly now out of the recent discussions that we define this as a bug-fix and can therefore include it in the next minor release?

yes, that was also my understanding.

The thing regarding the new property contributionType: This could be handled in another issue/PR I'd say

sounds good 👍

@tyrasd tyrasd modified the milestones: 2.0, 1.4 Feb 24, 2021
@FabiKo117 FabiKo117 removed the breaking This will introduce an incombability to previous versions. Documentation update compulsory label Mar 3, 2021
@FabiKo117 FabiKo117 force-pushed the fix-contrib-type-data-type branch from 260abad to 186055b Compare March 4, 2021 10:11
@FabiKo117 FabiKo117 added the waiting for review This pull request needs a code review label Mar 4, 2021
@bonaparten bonaparten self-requested a review March 4, 2021 10:32
@FabiKo117 FabiKo117 changed the title WIP: modify data type of contribution type modify data type of contribution type Mar 4, 2021
@FabiKo117 FabiKo117 merged commit 346cdb9 into master Mar 4, 2021
@FabiKo117 FabiKo117 deleted the fix-contrib-type-data-type branch March 4, 2021 10:52
@tyrasd tyrasd removed the waiting for review This pull request needs a code review label Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants