edi3 / edi3-codelists

GNU General Public License v3.0
0 stars 6 forks source link

Swagger reshaping proposal for recommendation 20 #6

Closed rgenoulaz closed 5 years ago

rgenoulaz commented 5 years ago

Reshaping proposal based on feedback and past experiences on designing referential API.

Hi, I’ve been thinking on the use cases that this API should answer to and I kinda re shaped it accordingly. Please let me know how you feel about it.

I guess that /quantityCategory and /unitofmeasure were eventually two ways of accessing to the same resource, being the list of units of measure, from a restful perspective at least.

So here are the main updates I made :

Please comment, react & challenge !

kshychko commented 5 years ago

@rgenoulaz , thanks a lot for your PR! Most of the changes are very reasonable and were on our to-do list, like updating parameters to camelCase and eliminating slashes from the parameters names.

I guess that /quantityCategory and /unitofmeasure were eventually two ways of accessing to the same resource, being the list of units of measure, from a restful perspective at least.

At the moment two different csv files are used for these two different resources and I think it's reasonable to keep both of them. I'm not an expert, but I can see that even a number of items in data/annexI.csv and data/annexII.csv is different. @cmsdroff , @Lewis-David-Wong could you please comment on this?

And some more feedback from me below:

Changed the name of the resource to make it a plural : unitsOfMeasure. Update all parameters to camelCase Changed the name of one parameter that had a slash in the middle, I’m afraid this could cause some trouble from an implementation perspective ( Level/Category  level). Also updated description to contain the old name as information. I also added some descriptions based on the documentation from UN/CEFACT

Happy with the changes listed above!

I removed technical IDs such as groupID (Identifier (numeric ID) of the unit of measure, within the group) groupNumber (Identifier (numeric ID) for the group/sector")

If there is no use in these IDs I agree that they can be removed, but we need to be confident in that before excluding them.

I added a path with the commonCode as path parameter

It seems to be a good choice for an ID for a unit of measure, so I'm happy with introducing this path parameter.

I removed some of the parameter from input such as commonCode or description, considering that we would rather search by name or sector rather than code (which would then be accessed from the other path directly)

I would keep description parameter, but happy to remove commonCode as it becomes a path parameter.

I added a specific input param for pagination and a specific error

I would opt for limit pagination scheme with offset and limit parameters.

@cmsdroff, I'd like to raise a GitHub issue per each point to discuss, that will help to keep the discussion threads organised. Do you have any objections?

cmsdroff commented 5 years ago

@rgenoulaz thanks for your pull request! :)

I agree with a lot of what has already been said by @kshychko so wont repeat, if its not mentioned below I'm in agreement!

I guess that /quantityCategory and /unitofmeasure were eventually two ways of accessing to the same resource, being the list of units of measure, from a restful perspective at least.

We tried to follow what was used in the existing recommendation text, this is why we included in the swagger spec, however it was purely to get something up for discussion, there are differences in the text provided in each excel sheet. From a transport and logistics perspective I am interested in Annex 2/3 only as I want the list of units of measure i.e. KGM and the descriptive text. Other users would have more use for the conversion data etc contained in annex 1 who may not be in transport and logistics i.e. scientific fields maybe? I think we should keep the two for now and discuss them with UN/CEFACT members who know more of the history, I've asked if there is a 'guru' for recommendation 20, as we don't yet have enough members with wide enough scope in our GitHub (but sure we will!)

I removed technical IDs such as groupID (Identifier (numeric ID) of the unit of measure, within the group) groupNumber (Identifier (numeric ID) for the group/sector") If there is no use in these IDs I agree that they can be removed, but we need to be confident in that before excluding them.

Agree, these for me have no meaning, but until we have consensus / confirmation I think they should stay, I'll ask @Lewis-David-Wong to do some reading on these and ask Sue Probert for guidance too.

I'd also suggest we drop status as this indicates what is going to be or has been deleted etc, this would be better handled in github as its a standard subversion feature, would you both agree @kshychko @rgenoulaz?

On the points we need to seek guidance on we should 100% raise a GitHub issue to keep track, happy for this to go ahead. Other than that great job and thanks for feedback :)

cmsdroff commented 5 years ago

@rgenoulaz also the descriptors/parameters of the data fields removed, agree they were not complete, but do you not think these are useful or do you have a preference for documentation ?

cmsdroff commented 5 years ago

Sue has given some points of clarification on the annex layouts, could be useful in understanding differences

The three annexes are structured accordingly: Annex I – Code elements listed by quantity category. This annex is normative and contains only level 1 and 2 entries.

Annex II – Code elements listed by name. This annex is informative and contains all level 1, 2 and 3 entries.

Annex III – Code elements listed by common code. This annex is informative and contains all level 1, 2 and 3 entries.

We can see this is why annex 2 and 3 are in same file, annex 3 essentially gives simplest set of codes, we know that all EDIFACT messages use these and so do WCO (World Customs Organisation) for example.

The above supports keeping them separate I feel

rgenoulaz commented 5 years ago

@cmsdroff @kshychko Thanks for feedback, and sorry for delay. if I wrap up :

Thank you all

Romain

kshychko commented 5 years ago

I created an issue on the items we are agreed on - #9 and some more on the questions to be discussed. I suggest closing this PR and opening a new one completing the items from #9.

rgenoulaz commented 5 years ago

New PR created for that matter with fewer updates :(