JPlag icon indicating copy to clipboard operation
JPlag copied to clipboard

Refactor CLI classes to package `de.jplag.cli`

Open JanWittler opened this issue 2 years ago • 4 comments

Currently, the CLI classes are in the same package as the JPlag core, i.e. de.jplag. However, as they serve a different purpose, they should belong to a different package. Thus, this PR refactors all classes of the /cli directory to be in package de.jplag.cli.

Additionally, the LanguageLoader is moved to the CLI package, as it should never be used by the Java programmatic API. Instead, there the language should be imported and directly instantiated.

⚠️ This change is API-breaking (as it removes the LanguageLoader from the core), so we must first adapt the branching behavior before merging this.

JanWittler avatar Sep 30 '22 13:09 JanWittler

CLI will not be published to maven central. As long as the arguments do not change, there should be no problems.

dfuchss avatar Sep 30 '22 14:09 dfuchss

CLI will not be published to maven central. As long as the arguments do not change, there should be no problems.

Unfortunately, the LanguageLoader is currently in the core and thus published to Maven central. As it is moved to CLI in this PR, this will remove public (though unintended) functionality from the core.

JanWittler avatar Oct 02 '22 12:10 JanWittler

I'm not sure whether the dynamic loading is never useful . Maybe that's something to discuss :) Since we provide the languages as services, we should also have an API to load them (I guess)

dfuchss avatar Oct 02 '22 12:10 dfuchss

I'm not sure whether the dynamic loading is never useful . Maybe that's something to discuss :) Since we provide the languages as services, we should also have an API to load them (I guess)

I did not see a use case where this functionality would become necessary as the language to test has to be specified anyways, and currently providing the language loader only enables the bad pattern of using the language loader + language identifier constant for loading the language instead of directly instantiating only the required language. For this reason, I moved the language loader to the cli package, thus making it inaccessible for the Java API itself. It might become useful when implementing a generic language frontend that selects from a set of language one language that matches the file extension. However, even for such a frontend we should discuss whether it is desirable to dynamically load all languages or rather instantiate the generic frontend with a set of supported languages.

JanWittler avatar Oct 13 '22 11:10 JanWittler

Ok for me.

dfuchss avatar Oct 13 '22 13:10 dfuchss

@JanWittler I've pushed one commit for the coverage-report here as well because before that was some kind of "implicit magic". Now it's explicitly stated in the pom.xml of the CLI

dfuchss avatar Oct 13 '22 13:10 dfuchss