Skip to content

Common 'area' data-type#315

Merged
rartych merged 3 commits intocamaraproject:mainfrom
tlohmar:main
Nov 27, 2024
Merged

Common 'area' data-type#315
rartych merged 3 commits intocamaraproject:mainfrom
tlohmar:main

Conversation

@tlohmar
Copy link
Contributor

@tlohmar tlohmar commented Oct 11, 2024

The PR adds a Area Data Type to CAMARA_common.yaml, which is copied from the location-retrieval API (Device Location). Variants of the Data Type are used by the geofencing-subscriptions API (Device Location) and the population-density-data API.

The PR is adding a consideration on self-intersecting polygons.

What type of PR is this?

  • documentation

What this PR does / why we need it:

The Area Data Type is used in multiple CAMARA APIs.

Which issue(s) this PR fixes:

Fixes #242

Does this PR introduce a breaking change?

  • Yes
  • No

Special notes for reviewers:

Changelog input

a _Area_ Data Type to CAMARA_common.yaml, copied from the _location-retrieval_ API (Device Location)

Additional documentation

This section is blank.

…retrieval API (Device Location).

Adding a consideration on self-intersecting polygons
Copy link

@chinaunicomyangfan chinaunicomyangfan left a comment

Choose a reason for hiding this comment

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

LGTM

patrice-conil
patrice-conil previously approved these changes Oct 24, 2024
Copy link
Contributor

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

LGTM.

PedroDiez
PedroDiez previously approved these changes Oct 28, 2024
Copy link
Contributor

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM

rartych
rartych previously approved these changes Nov 22, 2024
PedroDiez
PedroDiez previously approved these changes Nov 22, 2024
Copy link
Contributor

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

I'm still not a fan of the mappings for the discriminators (I prefer to use the names of the derived classes because it's easier for development) but it will be ok once the typo is fixed

minimum: 1

Polygon:
description: Polygonal area. The Polygon should be a simple polygon, i.e. should not intersection itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix typo "should not intersect itself"

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in c416430

@rartych rartych dismissed stale reviews from PedroDiez and themself via c416430 November 25, 2024 09:56
@rartych rartych requested a review from PedroDiez November 25, 2024 09:58
Copy link
Contributor

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM

@rartych rartych merged commit f157941 into camaraproject:main Nov 27, 2024
@hdamker
Copy link
Collaborator

hdamker commented Dec 6, 2024

Does this PR introduce a breaking change?

  • Yes
  • No

@rartych From Commonalities perspective it is correct that it isn't breaking change (as there wasn't previously a definition of Area). But for APIs who have already a different definition of Area and need to change over it might have the impact of a breaking change. We might need here a third category.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add an “Area” data-type into CAMARA_common.yaml

8 participants