-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add --normalize flag for Java #1155
Conversation
The way you documented the normalization flag indicates it is a global option. If your intent is a global option, that can be applied to all languages I would suggest adding it as a global option and just passing the value to the language for parsing. If the intent is a language specific option, that is available for all languages you could also add it to the global LanguageOptions class. Otherwise I could easily add a mechanism for shared options. Language specific options should only affect a single language module and should not be refereed to from the core module. |
I implemented an alternative way to add the flag. The branch is named normalize-flag-suggestion. |
Thanks, where can I view it? Your JPlag fork isn't public |
I committed it to your fork |
You can do that? I can't find it |
Sorry, I don't like forks very much, so I rarely use them. I forked your fork and pushed my branch here: https://github.com/TwoOfTwelve/JPlagMbrdl/tree/normalize-flag-suggestion |
Looks good, thanks 👍 |
But maybe normalization shouldn't be a language-specific option at all, since in principle any language can support it. Maybe it should just be a global option with a disclaimer. idk |
@@ -125,6 +125,9 @@ Advanced | |||
-x, --exclusion-file=<exclusionFileName> | |||
All files named in this file will be ignored in the | |||
comparison (line-separated list) | |||
|
|||
--normalize (Java only) Enable token string normalization, | |||
which can help detect certain cases of plagiarism. |
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.
This documentation does not fit the implementation. This implies, that --normalize is a global argument, but it is implemented as language specific.
@@ -67,6 +67,9 @@ Advanced | |||
-x, --exclusion-file=<exclusionFileName> | |||
All files named in this file will be ignored in the | |||
comparison (line-separated list) | |||
|
|||
--normalize (Java only) Enable token string normalization, | |||
which can help detect certain cases of plagiarism. |
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.
Same as above
|
||
public class JavaLanguageOptions extends LanguageOptions { | ||
public JavaLanguageOptions() { | ||
super(true); |
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.
This solution is fine, if we choose to implement --normalize as a language specific option.
@mbrdl we still have some open requests here. |
@@ -0,0 +1,38 @@ | |||
package de.jplag.options; | |||
|
|||
class DummyOption<T> implements LanguageOption<T> { |
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.
Javadoc to explain the purpose.
this.options = new ArrayList<>(); | ||
if (supportsNormalization) { | ||
var description = "Enable token string normalization, which can help detect certain cases of plagiarism."; |
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.
Avoid var, especially when String is nearly just as short.
Putting this off for now due to unresolved design decisions and lack of urgency |
todo: add cli tests for the flag |
# Conflicts: # languages/java/src/main/java/de/jplag/java/JavaLanguage.java
[JPlag Plagiarism Detector] Kudos, SonarCloud Quality Gate passed! |
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.
Minor comment
/** | ||
* New instance | ||
*/ |
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.
Document default option
@mbrdl this branch has some merge conflicts; could you resolve them :) |
This PR is on ice for now, we'll probably implement the option globally instead |
Adresses #1028. I'm not too happy with the way language-specific options work right now but I guess that's a topic for another day.