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

addition of parameter ‘unclipped’ #29

Merged
merged 3 commits into from
Aug 21, 2020
Merged

Conversation

FabiKo117
Copy link
Contributor

@FabiKo117 FabiKo117 commented Aug 19, 2020

implementing processing and adding it to the logic
adding it to the docs (swagger and sphinx)
refactoring boolean parameter usage
adding test
closes #20

implementing processing and adding it to the logic
adding it to the docs (swagger and sphinx)
refactoring boolean parameter usage
adding test
@FabiKo117 FabiKo117 added this to the 1.1 milestone Aug 19, 2020
@FabiKo117 FabiKo117 requested a review from tyrasd August 20, 2020 07:54
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

looks good. small suggestion: maybe we should put a comment in the code that the old (undocumented) variant with properties=unclipped is deprecated, so we don't forget to remove it in the next release(s). perhaps it's possible to use @Deprecated for that?

docs/endpoints.rst Outdated Show resolved Hide resolved
@tyrasd tyrasd added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 20, 2020
@FabiKo117
Copy link
Contributor Author

FabiKo117 commented Aug 20, 2020

looks good. small suggestion: maybe we should put a comment in the code that the old (undocumented) variant with properties=unclipped is deprecated, so we don't forget to remove it in the next release(s). perhaps it's possible to use @Deprecated for that?

I've tried this already, but can't find a proper position where to add the @deprecated annotation. Can you find one maybe?
Cause it's only relevant in line 464 in InputProcessor.java.

@tyrasd
Copy link
Member

tyrasd commented Aug 20, 2020

looks good. small suggestion: maybe we should put a comment in the code that the old (undocumented) variant with properties=unclipped is deprecated, so we don't forget to remove it in the next release(s). perhaps it's possible to use @Deprecated for that?

I've tried this already, but can't find a proper position where to add the @deprecated annotation. Can you find one maybe?
Cause it's only relevant in line 464 in InputProcessor.java.

I believe it should be possible if one rewrites the whole statement slightly, like this:

    for (String property : properties) {
      @Deprecated boolean oldUnclippedParameter = "unclipped".equalsIgnoreCase(property)
      if ("tags".equalsIgnoreCase(property)) {
        this.includeTags = true;
      } else if ("metadata".equalsIgnoreCase(property)) {
        this.includeOSMMetadata = true;
      } else if (oldUnclippedParameter) {
        this.unclipped = true;
      } else {
        throw new BadRequestException(ExceptionMessages.PROPERTIES_PARAM);
      }
    }

@tyrasd
Copy link
Member

tyrasd commented Aug 20, 2020

aah, regarding my comment above #29 (comment), I realize now that it could result in double-negatives sometimes: unclipped=false could be a bit confusing. Maybe turn the whole thing around, and just call this parameter clipGeometries (or so)?!

@FabiKo117
Copy link
Contributor Author

aah, regarding my comment above #29 (comment), I realize now that it could result in double-negatives sometimes: unclipped=false could be a bit confusing. Maybe turn the whole thing around, and just call this parameter clipGeometries (or so)?!

that's a good point yes.. then it would be better to call it "clipGeometries"

to better express the meaning and avoid confusion
adapting the naming and conditions in the processing as well
@FabiKo117 FabiKo117 merged commit a0bf957 into master Aug 21, 2020
@FabiKo117 FabiKo117 deleted the adding-param-unclipped branch August 21, 2020 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

finalize and document "unclipped geometry mode"
2 participants