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

Feature/normalization option #1479

Merged
merged 18 commits into from
Feb 16, 2024
Merged

Feature/normalization option #1479

merged 18 commits into from
Feb 16, 2024

Conversation

TwoOfTwelve
Copy link
Contributor

@TwoOfTwelve TwoOfTwelve commented Jan 13, 2024

Implemented the cli (and JPlag) option to enable normalization.
If normalization is enabled, but not supported for the selected language JPlag immediately throws an exception.

The help text also shows, if normalization is supported for a language:

  cpp         supports normalization: false
  cpp2        supports normalization: true
  csharp      supports normalization: false
  emf         supports normalization: false
  emf-model   supports normalization: false
  go          supports normalization: false
  java        supports normalization: true
  kotlin      supports normalization: false
  llvmir      supports normalization: false
  python3     supports normalization: false
  rlang       supports normalization: false
  rust        supports normalization: false
  scala       supports normalization: false
  scheme      supports normalization: false
  scxml       supports normalization: false
  swift       supports normalization: false
  text        supports normalization: false
  typescript  supports normalization: false

(relates to #1028)

@Kr0nox
Copy link
Member

Kr0nox commented Jan 13, 2024

Since the README and wiki contains the help text, this PR should probably updates those

@TwoOfTwelve TwoOfTwelve requested review from Kr0nox and tsaglam and removed request for Kr0nox January 23, 2024 15:19
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

A few minor issues here.

cli/src/main/java/de/jplag/cli/CliOptions.java Outdated Show resolved Hide resolved
GreedyStringTiling coreAlgorithm = new GreedyStringTiling(options);
ComparisonStrategy comparisonStrategy = new ParallelComparisonStrategy(options, coreAlgorithm);
// Parse and validate submissions.
SubmissionSetBuilder builder = new SubmissionSetBuilder(options);
SubmissionSet submissionSet = builder.buildSubmissionSet();
if (options.normalize() && options.language().supportsNormalization()) {
submissionSet.normalizeSubmissions();
Copy link
Member

Choose a reason for hiding this comment

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

Good for now, when we enable it for EMF, we need a language method that can be called for the normalization.

core/src/main/java/de/jplag/JPlag.java Outdated Show resolved Hide resolved
docs/1.-How-to-Use-JPlag.md Outdated Show resolved Hide resolved
# Conflicts:
#	core/src/main/java/de/jplag/JPlag.java
#	core/src/main/java/de/jplag/options/JPlagOptions.java
@tsaglam
Copy link
Member

tsaglam commented Feb 2, 2024

@TwoOfTwelve Do you want to make the changes regarding the language interface in this PR, or should I merge?

@TwoOfTwelve
Copy link
Contributor Author

I would like to do the changes to the Language interface here. I should get this done today.

@TwoOfTwelve
Copy link
Contributor Author

@tsaglam I have implemented a rough solution. Please take a look at it. I know it is still lacking all documentation, but I would like to know what you think before putting any more time into this.

Basically, in langauge-api/standardOptions the StandardOptionsLanguage and the NormalizatbleLanguage classes contain a bunch of ungly code. Also there will be runtime exceptions if the interface is used without the abstract class. I think it is worth it, because it is the most flexible solution and also gives us an infrastructure for more options shared between languages.

@tsaglam
Copy link
Member

tsaglam commented Feb 2, 2024

@TwoOfTwelve

Basically, in langauge-api/standardOptions the StandardOptionsLanguage and the NormalizatbleLanguage classes contain a bunch of ugly code. Also there will be runtime exceptions if the interface is used without the abstract class.

Agreed, that is not ideal from my point of view.

I think it is worth it, because it is the most flexible solution and also gives us an infrastructure for more options shared between languages.

I do not think so, as it introduces a lot of complexity just to prematurely plan for future use cases.
I think I prefer the lightweight option we discussed in the last meeting where we just provide a version of the parse method that supplies the language module with the information if normalization was enabled or not.

List<Token> parse(Set<File> files, boolean normalizationRequested) throws ParsingException;

default List<Token> parse(Set<File> files) throws ParsingException {
	parse(files, false);
}

This is minimally invasive and should not be API breaking through the default option.

@tsaglam tsaglam added enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change labels Feb 13, 2024
@Kr0nox Kr0nox added this to the v5.0.0 milestone Feb 13, 2024
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Just the minor issue we discussed regarding the language interface. Coverage is fine in this PR.

@tsaglam
Copy link
Member

tsaglam commented Feb 16, 2024

@TwoOfTwelve now you know why I was thinking about replacing as much code as possible in the Scala module with Java code! :D

Coverage is fine for me, spotless needs a minor fix.

Copy link

sonarcloud bot commented Feb 16, 2024

Quality Gate Failed Quality Gate failed for 'JPlag Plagiarism Detector'

Failed conditions
47.1% Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

@tsaglam tsaglam merged commit 7012aa4 into develop Feb 16, 2024
8 of 9 checks passed
@tsaglam tsaglam deleted the feature/normalizationOption branch February 16, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants