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

refactor: introduce ModificationStatus for better readability #3396

Merged
merged 10 commits into from
Jun 8, 2020

Conversation

monperrus
Copy link
Collaborator

No description provided.

@@ -133,29 +133,29 @@ protected void printSpaces(int fragmentIndex) {
* false if whole `fragment` is not modified.
* null if it is not possible to detect it here. Then it will be detected later.
Copy link
Collaborator

Choose a reason for hiding this comment

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

update comment, null value is not returned anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, thanks.

Comment on lines +23 to +30
public boolean toBoolean() {
if (this == MODIFIED) {
return true;
}
if (this == NOT_MODIFIED) {
return false;
}
throw new IllegalStateException();
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be abstract and all Enum constants implement this? Otherwise every new enum item would need a change in this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's not necessary to over-engineer, this enum is very unlikely to change

@@ -69,10 +69,10 @@ public boolean knowsHowToPrint(PrinterEvent event) {
}

@Override
protected Boolean isFragmentModified(SourceFragment fragment) {
protected ModificationStatus isFragmentModified(SourceFragment fragment) {
//we cannot fast detect if it is modified using our changeResolver.
//So return null. The code later will detect it including modified roles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

update comment, null value is not returned anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, thanks.

@monperrus
Copy link
Collaborator Author

  • some more simplification

@monperrus
Copy link
Collaborator Author

@nharrand ready for merge.

@nharrand nharrand merged commit e39950e into INRIA:master Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants