-
Notifications
You must be signed in to change notification settings - Fork 114
Fixed TODO: enum comment in Parser.java. Changed SyntaxAnalysisMode from int … #32
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
Fixed TODO: enum comment in Parser.java. Changed SyntaxAnalysisMode from int … #32
Conversation
1 similar comment
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.
Left a few comments for you in the review. Great work :)
} | ||
} | ||
|
||
public enum SyntaxAnalysisMode { |
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.
Lets pull this out into a separate file. Also, you can put the getSyntaxAnalysisModeFromInt
method into the enum itself.
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.
Done :)
EQUAL_OR_SHORTER_LENGTH | ||
} | ||
|
||
public static SyntaxAnalysisMode getSyntaxAnalysisModeFromInt(int syntaxAnalysisModeInt) { |
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.
Note: once this function is in the enum, we could implement it as:
if (syntaxAnalysisModeInt < 0 || syntaxAnalysisModeInt >= values().length) {
throw new IllegalArgumentException("Expected: 0, 1, or 2. Got: " + syntaxAnalysisModeInt);
} else {
return values()[syntaxAnalysisModeInt];
}
However, I like you implementation better, as it might be easier to read in the long run, so I suggest we keep it as it is.
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.
Okay :)
|
||
import static org.junit.Assert.assertEquals; | ||
|
||
public class ParserTest { |
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.
Reminder: Rename this test once the enum is in its own file.
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.
Done :)
return recognizedPlate.getString(); | ||
} | ||
|
||
public String parse(RecognizedPlate recognizedPlate, SyntaxAnalysisMode syntaxAnalysisMode) { |
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 changes one of the methods in the "public API" (there is no such thing currently, but I suspect people use the method in their code). We should be able to do this change for the next major release, but we should document it somehow. I added the following file: docs/upgradeRecipes/upgradeRecipe-2.0.0.adoc
. It will be of great help to all users if they wish to upgrade in the future.
Just add something along the lines of
=== `Parser.parse` method parameters
The method `String Parser.parse(RecognizedPlate, int)` changed to
`String Parser.parse(RecognizedPlate, SyntaxAnalysisMode)`.
Workaround: `parse(plate, syntaxAnalysisModeInt)`
-> `parse(plate, SyntaxAnalysisMode.getSyntaxAnalysisModeFromInt(syntaxAnalysisModeInt))`
To the "Minor API changes" part of that file.
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.
Done :)
…for the change to Parser.parse. Pulled SyntaxAnalysisMode into its own file and moved getSyntaxAnalysisModeFromInt into this file. Renamed ParserTest to SyntaxAnalysisModeTest to reflect this change.
1044ce4
to
9feccd2
Compare
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.
Thanks a lot! :)
…to enum.