-
Notifications
You must be signed in to change notification settings - Fork 18
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
Creating three packages and profiles for code generation #22
base: master
Are you sure you want to change the base?
Conversation
language is not relevant for the user
add continuous integration
@radissoa @AlberTadrous @joephayes any review :) |
.../test/java/semanticstore/ontology/library/generator/generators/java/OLGAJavaVersionTest.java
Show resolved
Hide resolved
@AlberTadrous there are two tests that are failing but not sure why yet, as I didn't touch the java generator. |
@@ -28,6 +28,11 @@ | |||
import java.util.Map; | |||
import org.codehaus.plexus.util.xml.pull.XmlPullParserException; | |||
import semanticstore.ontology.library.generator.exceptions.InvalidUriException; | |||
import semanticstore.ontology.library.generator.generators.AbstractGenerator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to make these plugins that are loaded at runtime? It would make it easier for a person adding a new generator...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joephayes
I actually started using Dagger for dependency injection as you are pointing out.
Then I realized others might not use it to extend OLGA.
I think it makes sense to seperate them and run the tests of code generation for all three.
But the compilation
tests we can't assert unless the underlying dependencies are installed.
thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joephayes friendly ping :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charbull Sorry for the delay!
At a high level, I'm thinking in terms of modules (plugins). OLGA would maintain a registry of modules and load the code as needed.
What if OLGA defined these as service interfaces and then the Python, C# and Java generators can be implemented as services? OLGA could then use the Java ServiceLoader to load the desired implementations.
If OLGA adopted this model, the code for the different generators would move into separate repos and the compilation tests would move as well.
It would also give a definitive answer to the question "how do I add a new generator?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charbull - is this something you want to discuss more? Maybe I should open a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joephayes I think it makes sense, I will start looking at it.
For the RDF python generator do you have some bandwidth to collaborate in co-development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charbull - A colleague of mine and I will have some bandwidth towards the end of this month and we could work on the generator for RDF Python then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great !
Will send another PR for the service loader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great! Looking forward to it!
The current state of OLGA requires the users to install dotnet, and python dependencies which requires unnecessary effort specially when the user is for example interested only in Java code generation or Csharp.
This PR proposes the following:
(two tests are not passing so far: testImportSaref and testMergedSaref) in OLGAJavaTest, commented for now, I will resolve the issue after this PR). # #