-
Notifications
You must be signed in to change notification settings - Fork 6
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
[API] Provide a better API #136
Conversation
@dvojtise would you have the time to take a look at this PR before I squash & merge the commits? I'd appreciate a review. |
Because: - some tasks are common and repeated across the codebase but no API is available, leading to code duplication - the purpose of some internal classes is unclear Changes: - the new IAleEnvironment interface represents a coherent set of metamodels and ALE behaviors - the new IAleInterpreter interface represents an interpreter able to evaluate ALE expressions - rename AstBuilder and ModelBuilder as BehaviorsParser and AntlrAstToBehaviorsAstAdapter - document unclear classes - removed the org.eclipse.emf.ecoretools.ale bundle (and move content to core bundle) - turn the org.eclipse.emf.ecoretools.ale.tests plug-in into a plug-in fragment
Because: - some tasks are common and repeated across the codebase but no API is available, leading to code duplication - the purpose of some internal classes is unclear Changes: - the new IAleEnvironment interface represents a coherent set of metamodels and ALE behaviors - the new IAleInterpreter interface represents an interpreter able to evaluate ALE expressions - rename AstBuilder and ModelBuilder as BehaviorsParser and AntlrAstToBehaviorsAstAdapter - document unclear classes - removed the org.eclipse.emf.ecoretools.ale bundle (and move content to core bundle) - turn the org.eclipse.emf.ecoretools.ale.tests plug-in into a plug-in fragment Signed-off-by: Emmanuel Chebbi <emmanuel.chebbi@outlook.fr>
...f.ecoretools.ale.core/src/org/eclipse/emf/ecoretools/ale/core/env/impl/DslConfiguration.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.emf.ecoretools.ale.ide/src/org/eclipse/emf/ecoretools/ale/ide/env/Normalized.java
Outdated
Show resolved
Hide resolved
...oretools.ale.core/src/org/eclipse/emf/ecoretools/ale/core/env/impl/MadeUpAleEnvironment.java
Outdated
Show resolved
Hide resolved
I think that the prefix is short enough to keep it. Additionally, ALE is one of the meta-approach used in GEMOC, and we may have to mix approaches, in such situation, having clear class names will help (this is always a pain in java to have 2 classes from different packages that have the same name because java doesn't provide local rename mechanism such as other languages) IMO, every class that might be used from outside (ie not internal) should keep the Ale prefix |
...pse.emf.ecoretools.ale.ide/src/org/eclipse/emf/ecoretools/ale/ide/project/impl/AleAware.java
Outdated
Show resolved
Hide resolved
...ecoretools.ale.core/src/org/eclipse/emf/ecoretools/ale/core/interpreter/IAleInterpreter.java
Show resolved
Hide resolved
Globally the proposed changes make sense and improve the API (only some class names that could be improved) once accepted, I would appreciate a PR in https://github.com/eclipse/gemoc-studio-execution-ale/ to report these changes to the GEMOC ALE engine that heavily use this API (I quickly tried renaming, but I think it could also be simplified thanks to the API addition you provided) |
* DslConfiguration -> FileBasedAleEnvironment * InMemoryAleEnvironment -> PathsBasedAleEnvironment * MadeUpAleEnvironment -> RawEnvironmentBasedAleEnvironment * Normalized -> WithAbsoluteBehaviorPathsAleEnvironment * AleAware -> AleProject Signed-off-by: Emmanuel Chebbi <emmanuel.chebbi@outlook.fr>
I think this PR can now be merged; @dvojtise are you OK with the changes? |
yes, looks good to merge :-) |
this file was moved/renamed in #136 Signed-off-by: Didier Vojtisek <didier.vojtisek@inria.fr>
this file was moved/renamed in #136 Signed-off-by: Didier Vojtisek <didier.vojtisek@inria.fr>
Contributes to #123
Changes
org.eclipse.emf.ecoretools.ale
plug-inorg.eclipse.emf.ecoretools.ale.tests
plug-in into a plug-in fragment to ease testing of internal classesHow to review
This PR impacts a lot of files but that's mainly because I renamed some classes and moved others. There's actually almost no change in features: I mainly introduced new interfaces (
IAleInterpreter
) and enforced the use of others (IAleEnvironment
). Both automatic and manual testing have failed to find bugs so I'm pretty confident in not having introduce important regressions.To review important changes I would advise to:
IAleEnvironment
AbstractAleEnvironment
AleInterpreter
BehaviorsParser
(exAstBuilder
) andAntlrAstToBehaviorsAstAdapter
(exModelBuilder
)org.eclipse.emf.ecoretools.ale.core
API
Lots of interfaces and classes are prefixed by
Ale
. It feels redundant and I can't decide whether it's worth it.See also unit tests for examples.
Task 1: create a new ALE project
Task 2: retrieve an existing ALE project
Task 3: manipulate an ALE environment
Task 4: manipulate ALE classes
The
IBehaviors
interface eases manipulation of dynamic ALE elements.Task 5: execute an ALE program
The environment itself is
AutoCloseable
and responsible for releasing resources (seeAbstractAleEnvironement::close
for the actual implementation).