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.

Sign up for GitHub

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

Jump to bottom

Refactor CLI classes to package de.jplag.cli #716

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Refactor CLI classes to package de.jplag.cli #716

merged 4 commits into from
Nov 7, 2022

Conversation

JanWittler
Copy link
Contributor

@JanWittler JanWittler commented Sep 30, 2022 •
edited
Loading

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.

@JanWittler JanWittler requested a review from a team September 30, 2022 13:56
@JanWittler JanWittler added the major Major issue/feature/contribution/change label Sep 30, 2022
@dfuchss
Copy link
Member

dfuchss commented Sep 30, 2022

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

dfuchss
dfuchss reviewed Sep 30, 2022
View reviewed changes
Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question :)

endtoend-testing/pom.xml Show resolved Hide resolved
@JanWittler JanWittler added enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change and removed major Major issue/feature/contribution/change labels Oct 2, 2022
@JanWittler
Copy link
Contributor Author

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.

@dfuchss
Copy link
Member

dfuchss commented Oct 2, 2022

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)

@JanWittler
Copy link
Contributor Author

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.

@dfuchss
Copy link
Member

dfuchss commented Oct 13, 2022

Ok for me.

@dfuchss
Copy link
Member

dfuchss commented Oct 13, 2022

@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

@JanWittler JanWittler changed the base branch from main to develop October 14, 2022 14:43
@tsaglam tsaglam added this to the v4.0.1 milestone Oct 14, 2022
@JanWittler JanWittler marked this pull request as ready for review October 17, 2022 07:55
@JanWittler JanWittler force-pushed the cli-package branch 2 times, most recently from d216592 to b5c0c2d Compare October 27, 2022 13:56
@JanWittler JanWittler mentioned this pull request Oct 28, 2022
Merged
@sonarcloud
Copy link

sonarcloud bot commented Oct 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

73.1% 73.1% Coverage
0.0% 0.0% Duplication

@tsaglam tsaglam merged commit 683d785 into develop Nov 7, 2022
@tsaglam tsaglam deleted the cli-package branch November 7, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfuchss dfuchss dfuchss left review comments

@tsaglam tsaglam tsaglam approved these changes

Assignees
No one assigned
Labels
enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@JanWittler @dfuchss @tsaglam