camaraproject / DeviceLocation

Repository to describe, develop, document and test the DeviceLocation API family
Apache License 2.0
21 stars 32 forks source link

Area type case constituency - Circle or CIRCLE #131

Closed bigludo7 closed 9 months ago

bigludo7 commented 9 months ago

Problem description In Geofencing we have:

  Area:
      type: object
      properties:
        areaType:
          $ref: "#/components/schemas/AreaType"
      required:
        - areaType
      discriminator:
        propertyName: "areaType"
        mapping:
          CIRCLE: "#/components/schemas/Circle"

   AreaType:
      type: string
      description: |
        Type of this area.
        CIRCLE - The area is defined as a circle.
      enum:
        - CIRCLE

in Location Verification we have:

Area:
      properties: 
        areaType:
          type: string
          description: Type of this area.
      required:
        - areaType
      discriminator: 
        propertyName: areaType
        mapping: 
          Circle:  "#/components/schemas/Circle"

In Location Retrieval

   Area:
      type: object
      required:
        - areaType
      properties:
        areaType:
          description: type of the area (like Circle or a Polygon)
          type: string
      discriminator: 
          propertyName: areaType
          mapping: 
            Circle:  "#/components/schemas/Circle"
            Polygon: "#/components/schemas/Polygon"

We have some inconsistencies here

  1. use of enumeration for areaType
  2. value in UPERCASE or CamelCase

Expected behavior Align design within our API. Probably we can align geofencing API with the 2 other APIs Seems to me that alignement on case should be done for this RC (I can do it)

Alternative solution

Additional context

akoshunyadi commented 9 months ago

Yes, we should definitely harmonize it. But isn't enum the better/safer way? The "rc" is already released, so this one could go into "rc2". But with that I would wait for more feedback.

bigludo7 commented 9 months ago

Thanks @akoshunyadi Same view for the enum. Yes ok let's wait some time to get more feedback from implementation.

jlurien commented 9 months ago

We have to decide on the same approach in all 3 APIs for sure. The one in geofencing, defining an enum is the best one to me. We usually use UPPER_CASE for enums, although this is not strictly stated in the guidelines, but I would favor it here.

About releasing, I would fix it in the current release-0.2.0-rc branch and in main as well., Creating Github releases and branches per fix is too much to me. This RC period is exactly to report bugs and fix the RC, so we may expect more fixes coming. We may rename the version in the spec file to -rc2 after the fix.

akoshunyadi commented 9 months ago

I'm fine with upper case for enums too.

About the releasing:

  1. The most important is that we won't have different versions of the file with the same version number
  2. A quick fix on the -rc branch (with version increment) is ok for 1-2 fixes, but even then you will have a different "rc" package, depending on when you pull it. I mean 2 days later the same link provides a different version. It might create confusion. If we have many such fixes, with many different versions of a file, that will be hard to follow by the developer teams. Because of these I would prefer to have a bundled -rc2.
jlurien commented 9 months ago

First, let's agree on the fix. @bigludo7 are you OK with the UPPER_CASE enums?

bigludo7 commented 9 months ago

@jlurien Yes !

jlurien commented 9 months ago

OK, I'll make a PR with it