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

Changes in API-interfaces regarding return types #1034

Open
sszuev opened this issue Dec 25, 2021 · 4 comments
Open

Changes in API-interfaces regarding return types #1034

sszuev opened this issue Dec 25, 2021 · 4 comments

Comments

@sszuev
Copy link
Contributor

sszuev commented Dec 25, 2021

I think, it would be better to replace all the concrete modifiable classes from interfaces with interfaces as well.
Basically, it's about the return types mentioned in the org.semanticweb.owlapi.model.OWLOntologyManager, i.e. about the followings:

  • org.semanticweb.owlapi.model.OntologyConfigurator (ONT-API has extended configuration)
  • org.semanticweb.owlapi.model.OWLOntologyLoaderConfiguration
  • org.semanticweb.owlapi.model.OWLOntologyWriterConfiguration
  • org.semanticweb.owlapi.util.PriorityCollection (or replace it with java.util.Collection)

I think this also would be more correct in terms of architecture\design:
it is better when interface methods do not return mutable non-final concrete classes.

In order not to interfere with the current client code, this could be done somewhere in version 5.2+
v6.x.x, I think, implies more deep changes, while these proposed changes may not be even visible to end-users (to notice, it is need to have code calling constructors directly).
And also, generally speaking, it would be better to make a generic mechanism to retrieve\set configuration that would not depend on implementation at all.


Originally it was posted here: https://github.com/orgs/owlcs/teams/ontapi-contributors/discussions/1

@ignazio1977
Copy link
Contributor

a generic mechanism to retrieve\set configuration that would not depend on implementation at all

I like that idea. What would this look like?

@sszuev
Copy link
Contributor Author

sszuev commented Jan 22, 2022

I was thinking of something like this:

    interface OWLOntologyManager {
        OntologyConfig getOntologyConfigurator();
    }
    interface OntologyConfig {

        OntologyConfig setBoolean(Key key, boolean value);

        boolean getBoolean(Key key);

        @Deprecated // ?
        default OntologyConfig setAcceptingHTTPCompression(boolean b) {
            return setBoolean(OWLAPISettings.BOOLEAN_ACCEPT_HTTP_COMPRESSION, b);
        }

        @Deprecated // ?
        default boolean shouldAcceptHTTPCompression() {
            return getBoolean(OWLAPISettings.BOOLEAN_ACCEPT_HTTP_COMPRESSION);
        }

        interface Key {
        }
    }
    enum OWLAPISettings implements OntologyConfig.Key {
        BOOLEAN_ACCEPT_HTTP_COMPRESSION,
    }

I think, such a configuration should be much easier to override and extend across implementations.
(to be more specific, I should probably prepare a PR ?)

@ignazio1977
Copy link
Contributor

Why not simplify further and just have a Map<String, Object>? ConfigurationOptions can act as the list of known properties and be easily adapted to expose the property name (it already has that information to look up properties from property files and system properties), and any implementations specific property can be added without impact. ConfigurationOptions would just be there for defaults and to avoid mistyping property names.

@sszuev
Copy link
Contributor Author

sszuev commented Jan 22, 2022

Why not simplify further and just have a Map<String, Object>? ConfigurationOptions can act as the list of known properties and be easily adapted to expose the property name (it already has that information to look up properties from property files and system properties), and any implementations specific property can be added without impact. ConfigurationOptions would just be there for defaults and to avoid mistyping property names.

I think that wouldn't be very user-friendly:

  • I think, java.util.Map is inconvenient structure here and even dangerous: we should not encourage clients to use raw-types: in v5 config setting values can be Boolean, String, Enum, and even arbitrary objects (e.g. in ontapi).

  • There are now three different configuration types: one mutable manager-wide config and a pair of load & store immutable configs for every ontology. So the current behavior is quite complicated. I'm not sure it is possible to re-implement it using only JDK Maps (or even Properties)

  • Also, any object in OWLAPI implements Serializable, that also may lead to some difficulties for end-users.

  • In contrast to this, the pattern above is safe and does not require any change in the client code (unless they explicitly use constructors to initialize that configs - which, I think, should be rare). Also, it can be applied right now with comparably little efforts.

But on the other hand, I agree that the configuration management code is overly complicated, and switching to use java.util.Map would partially solve this problem.

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

2 participants