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

Splitting complex types, event-based types and code-based types (enums) into saparate sub-folders #66

Closed
ahokkonen opened this issue Feb 11, 2020 · 18 comments
Assignees

Comments

@ahokkonen
Copy link
Contributor

ahokkonen commented Feb 11, 2020

@alamers @erwinspeybroeck @cookeac
Just asking for your opinion if you see a need for splitting different types - complex vs code-based types (enums) into separate folders for easier navigation inside code?

example structure:

-types

  • icarParentageType.json
  • icarSireRecommendationType.json
  • icarResourceCollection.json
  • etc.

-enums

  • icarReproHeatSignType.json
  • icarReproHeatDetectionMethodType.json
  • etc.

icarAnimalCoreResource.json
icarEventCoreResource.json
icarReproHeatEventResource.json
etc.

@cookeac
Copy link
Collaborator

cookeac commented Feb 11, 2020

I did think about this too, but I felt that it made the $ref inclusion simpler (removing the need to get relative paths right) and that the naming convention was sufficient. However, I'm interested in others' feedback.

@alamers
Copy link
Collaborator

alamers commented Feb 12, 2020

No strong opinion; naming should be sufficient as Andrew states. I don't know if the relative paths will be an issue, I suspect we'll have those sooner or later.

@erwinspeybroeck
Copy link
Collaborator

It would make the long list we are getting now more clear and structured. Don't know what the effect might be on the $ref.

@ahokkonen
Copy link
Contributor Author

Yes, I totally agree with you on naming convention - it should be sufficient.
My question was only from developer POV working with code sources :) On some point - when more and more new messages will be included into standard - some sort of structure reconstruction will be unavoidable I guess.

On code-based types (enums) I also noticed that most of them are extracted into own separate files (like icarRegistrationReasonType, icarReproHeatIntensityType, etc), but also few wich are declared inside complex types - for example bottleIdentifierType (enum type) inside icarAnimalMilkingSampleType and icarQuarterMilkingSampleType. If the meaning for bottle identifier is same in both structures it is probably good to exctract into shared type (separate file icarBottleIdentifierType.json)?

@ahokkonen
Copy link
Contributor Author

It would make the long list we are getting now more clear and structured. Don't know what the effect might be on the $ref.

of course, all references ($ref) should be updated if resource location is changed (moved to sub-folder).

@ahokkonen
Copy link
Contributor Author

ahokkonen commented Feb 25, 2020

One more thing we could potentially discuss is to have a separated schemas for Types and Resources.

Now we have all types which we expose on API layer defined as "Resource" (icarReproPregnancyCheckEventResource, icarReproInseminationEventResource etc), but technical they are just Types provided with some additional source related information like meta data, self identifier through "icarResource" or "icarEventResource" inheritance. Types that we are not exposing at the moment on API are defined as "Type". If one day we decide to expose one of these existing types on API layer we'll need to modify/rename schema to "Resource" and also correct all references in another types if there are any references to origin (might be tricky if there are lot of them).

Solution would be to separate Resource and Type on schema level (icarReproInseminationEventResource as an example):

  • create separate type icarReproInseminationType (move all properties from origin)
  • modify resource icarReproInseminationEventResource to inherit from both icarEventResource and new icarReproInseminationType and remove all properties
{
  "type": "object",
  "description": "Event for recording natural or artificial insemination, including embryo transfer.",
  "required": [
    "id",
    "animal",
    "eventDateTime",
    "inseminationType"
  ],
  "allOf": [
    {
      "$ref": "icarEventCoreResource.json"
    },
    {
      "$ref": "icarReproInseminationType.json"
    }
  ]
}

This concept will probably bring more flexibility in creating references/dependencies between types and more easy way to introduce types as a resources (when needed) on url schema.

@cookeac
Copy link
Collaborator

cookeac commented Feb 28, 2020

@ahokkonen's suggestion in #66 (comment) is nice.

Thinking about the folder structure suggested in #66 (comment), and realising that we will soon use Branches for versioning, should we replace the Release Candidate Messages folder with folders for

  • url-schemes
  • resources
  • types
  • enums
  • examples

Most references would then look like "$ref" : "./types/icarParentageType.json"

@alamers
Copy link
Collaborator

alamers commented Feb 28, 2020

I agree, the subfolders here make sense to me. Nice suggestion!

@ahokkonen
Copy link
Contributor Author

Ok, great! Then I will start to reorganize project according proposed folder structure. I hope these changes are ready next week :)

@erwinspeybroeck do you have Lactation API ready for merging into master? Should I wait before you do that?

Or if there anyone else having things to merge before I start doing massive changes into structure?

@erwinspeybroeck
Copy link
Collaborator

Anton, I have nothing ready - only the definition in the issue.
I can probably work on that on 5/3 - I can try to do it in the new way, if the folders are there?

@ahokkonen
Copy link
Contributor Author

@erwinspeybroeck ok, I'll try to be ready with my task before 5.3

@ahokkonen
Copy link
Contributor Author

@ahokkonen's suggestion in #66 (comment) is nice.

Thinking about the folder structure suggested in #66 (comment), and realising that we will soon use Branches for versioning, should we replace the Release Candidate Messages folder with folders for

  • url-schemes
  • resources
  • types
  • enums
  • examples

Most references would then look like "$ref" : "./types/icarParentageType.json"

@cookeac @alamers @erwinspeybroeck should all deprecated folders (DataLink, JoinData) be removed as part of that change? Or should I keep them for a while?

@alamers
Copy link
Collaborator

alamers commented Feb 28, 2020

I'd say, remove them :). Right now, they are confusing and not in use. We'll always have the history if we need them for some reason.

@ahokkonen
Copy link
Contributor Author

@cookeac @alamers @erwinspeybroeck asking for your opinion once again about change described in #66 (comment).

For example, when extracting icarReproInseminationType as separate type should "eventDateTime" field retain its original name or should it be renamed to "inseminationDateTime" instead? Same in icarReproParturitionType -> parturitionDateTime, etc...

@alamers
Copy link
Collaborator

alamers commented Feb 28, 2020

If I understand correctly, the eventDatetime is inherited from the icarEventCoreResource. Those fields are intended as generic fields that apply for every event. I don't think it makes much sense to rename them as that would defeat the whole generalization of the event. (Idea would be that you could do some generic event handling based on those fields.)

Or do I understand you wrong?

@ahokkonen
Copy link
Contributor Author

If I understand correctly, the eventDatetime is inherited from the icarEventCoreResource. Those fields are intended as generic fields that apply for every event. I don't think it makes much sense to rename them as that would defeat the whole generalization of the event. (Idea would be that you could do some generic event handling based on those fields.)

Or do I understand you wrong?

Yes, that is true, that is why I've decided not to hurry with that change.

Just realized that only by exposing icarReproInseminationEventResource properties into separate icarReproInseminationType makes no sense, because also parts (animal, eventDateTime, location, etc) from icarEventCoreResource should be taken into that new type. Without that icarReproInseminationType would be incomplete and useless due missing mandatory data fields.
So maybe my proposal needs to be reviewed...

@ahokkonen
Copy link
Contributor Author

ahokkonen commented Apr 17, 2020

Few things to mention is that code generators are not very happy with splitting main url scheme into parts. I tried to split paths, responses and parameters into separate sub-files and code get errors during generation. Another thing is that paths elements cannot be extracted into pieces (separated files for get/post or milk/reproduction) - not supported by swagger nor generators.

@cookeac
Copy link
Collaborator

cookeac commented Jul 30, 2020

I think the main changes contemplated by this Issue have all been completed, so I will close it. If you would like to address any of the other comments or questions please reopen this or create another linked issue.

@cookeac cookeac closed this as completed Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants