-
Notifications
You must be signed in to change notification settings - Fork 12
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
Rewrite object/restriction visitor to handle all complex scenarios. Some fixes. Add new configuration options. #182
base: master
Are you sure you want to change the base?
Conversation
…to non-array (and required)
Thanks @cweedall, |
I thought about treating it as a fork instead, because it is difficult to compare code-wise. There are some small things that I added along the way, such as the configuration flags, that could be removed - but these changes also are so small that it would not really impact the big-ness of the change. I have not been able to think of a good way to split this up to have it reviewed in several steps. Part of the problem was that |
@dgarijo I started looking into breaking this down into smaller pieces. My impression so far is that it would take a lot of work and probably take longer to separate than these changes took. I'll mark it ready to review. But if you and the other repo owners decide it is not reviewable or prefer not to merge, I will understand. In that case, I will set it up as a fork and perhaps it could be revisited in the future. |
That's ok, I will have a look soon |
I bumped a major version (4.0.0) due to the combination of updates.
Bumped some package versions to the newest version.
Config options + documentation:
New option to generate JSON instead of YAML. (I thought about trying to support JSON as a config file... but it's more in-depth than I would like to go).
New kebab case flag (current/default is flatcase - all lowercase and no spaces; kebab case is industry standard for endpoint names).
New inheritance reference flag - if true, class will inherit via allOf reference to a super/parent class.
In general, removed references to class member variables and referenced them via a method. A mixture of both was in use before and it's simply easier to stay consistent, especially for a somewhat smaller codebase. Wherever I noticed a member variable reference in a class, I added
this.
for clarity. A somewhat stylistic choice, but I shifted tocamelCase
and away fromsnake_case
(the use of camel case is more prevalent in my experience, and soe this was more of a general industry consistency decision.). A shift to usingSet
s instead ofList
s because we shouldn't be using lists where duplicates of the same thing appear (owlapi generally does this... but for some reason they allow/use Lists for certain things like allOf, anyOf, etc. which I find a bit weird...).I think I accidentally broke the description getter in the previous update. This is now fixed. Class and property descriptions look good again. I also added the DSTERMs namespace to get descriptions defined with it which were not previous gotten.
Restructured the property mappers. Except for schema object/data property schema creation, make everything static. New generic
MapperProperty
for common functionality of object/data properties. Extend that class byMapperObjectProperty
andMapperDataProperty
. Avoid confusion switch cases. Use individual static adder/setter methods to modify schema details (e.g. cardinality) and handle different data constructs (e.g. oneOf, allOf, anyOf, etc.).Update
MapperOperation
to use links from the endpoint description to the associated model schema. Some media type updates. Some placeholder commented code for example generation. Currently, each endpoint now has a reference to the model schema that represents it. Technically, this is invalid, so an exception in the validator was added. It actually generates nicely in Swagger's UI and makes the process much easier, since we don't need to worry about determing example values for a property. I think(?) this is a bug somewhere and either the validator is wrong or the UI generation is wrong (and may get fixed in the future?).The
Mapper
class was cleaned up a bit and directly calls theObjectVisitor
(previouslyRestrictionVisitor
).The
MapperSchema
class mostly duplicated work thatMapper
was doing or that theObjectVisitor
/RestrictionVisitor
should be doing. A majority was moved toObjectVisitor
because it made the most sense and was supported under the abilities ofOWLObjectVisitor
. This class was deleted after moving the functionality.As noted above,
RestrictionVisitor
was renamed toObjectVisitor
. This was partly done because it was already extendingOWLObjectVisitor
and to keep the naming more consistent. As I was comparing withMapperSchema
I noticed that it made more sense to move a lot of the looping and searching for domains/ranges intoObjectVisitor
because they were tightly-coupled. The idea was to get each property and then do restrictions, where applicable. In some cases, getting the restrictions and getting the property were the same (and therefore, the process duplicated). This could occur, for example, if an ontology defines restrictions for a property and does not define any range for the property.I noticed that inheritance of object/data properties did not seem to work correctly. If you have a super property, for example, with a domain and the subproperty has a range, the subproperty would not correctly be generated in the OpenAPI spec. The object properties required the
reasoner
to calculate this things. It was a bit weird, but was done by calculating direct subproperties of theowl:topObjectProperty
, keeping track of any domains/ranges and then traversing deeper, if there are any deepar subproperties.