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

Fix saving triggers metadata changed #9165

Merged
merged 3 commits into from
Sep 20, 2022
Merged

Fix saving triggers metadata changed #9165

merged 3 commits into from
Sep 20, 2022

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Sep 19, 2022

Fixes #9159

Generate new equals, hashcode for FieldFormatterCleanup and toString methods

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Fixes #9159

Generate new equals, hashcode for FieldFormatterCleanup and toString methods
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 19, 2022
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

LGTM!

To cover cases when new fields are added to the classes, maybe, we should add test cases with deepEquals of AssertJ - https://stackoverflow.com/a/9633089/873282

Note to self: Testing equality by reflection is not an option, as a) it used reflection and b) is less performant: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/EqualsBuilder.html#reflectionAppend-java.lang.Object-java.lang.Object-

Comment on lines -108 to 117
public boolean equals(Object o) {
if (this == o) {
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (o instanceof FieldFormatterCleanup) {
FieldFormatterCleanup that = (FieldFormatterCleanup) o;
return Objects.equals(field, that.field) && Objects.equals(formatter, that.formatter);
if (!(obj instanceof FieldFormatterCleanup)) {
return false;
}
return false;
FieldFormatterCleanup other = (FieldFormatterCleanup) obj;
return Objects.equals(field, other.field) && Objects.equals(formatter, other.formatter);
}
Copy link
Member

Choose a reason for hiding this comment

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

The new code seems to be the equial to the old one. ^^.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just used eclipse to regenerate it to have it consistent with others

Comment on lines +209 to +230

@Override
public int hashCode() {
return Objects.hash(actions, enabled);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (!(obj instanceof FieldFormatterCleanups)) {
return false;
}
FieldFormatterCleanups other = (FieldFormatterCleanups) obj;
return Objects.equals(actions, other.actions) && (enabled == other.enabled);
}

@Override
public String toString() {
return "FieldFormatterCleanups [enabled=" + enabled + ", actions=" + actions + "]";
}
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for putting in the implementations here. I googled and found

By default, two objects are equal if and only if they are refer to the same memory location

@koppor koppor merged commit 7dcdd61 into main Sep 20, 2022
@koppor koppor deleted the fixMetadataDiff branch September 20, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignore unnecessary library modification notifications?
2 participants