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

Remove condition "If ISBN is equal means it's a duplicate" in "duplicate check" #11191

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

AbdAlRahmanGad
Copy link
Contributor

@AbdAlRahmanGad AbdAlRahmanGad commented Apr 14, 2024

Refs #8885

The reason of the problem is that in the 'duplicate check' if the "ISBN" is equal the entries are marked as duplicates
but that's not true in all cases. See #9769 (comment).

What I did was remove the condition and add weight to the field. The weight should be revised as I gave it a random value to demonstrate the idea.

Mandatory checks

  • 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.

@@ -65,6 +64,7 @@ public class DuplicateCheck {
DuplicateCheck.FIELD_WEIGHTS.put(StandardField.NOTE, 0.1);
DuplicateCheck.FIELD_WEIGHTS.put(StandardField.COMMENT, 0.1);
DuplicateCheck.FIELD_WEIGHTS.put(StandardField.DOI, 3.);
DuplicateCheck.FIELD_WEIGHTS.put(StandardField.ISBN, 2.5);
Copy link
Member

Choose a reason for hiding this comment

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

Does the modification also work without this addition? Reading the comment, this adds some duplicate check for articles having the same ISBN.

I think, it's good to keep, but I am wondering... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it works without this addition. Should I remove it for now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, remove it. We can add if we have more tests for similar entries... - I think, if entries are different, the same ISBN could make it the same. However, I am not sure about the algoirthm. Is it typos at the author? If same ISBN, the entries are same? If no ISBN, not same? That feels strange. -- Thus, remove.

@@ -525,7 +524,9 @@ void compareOfTwoEntriesWithSameContentAndMixedLineEndingsReportsNoDifferences()
assertTrue(duplicateChecker.isDuplicate(entryOne, entryTwo, BibDatabaseMode.BIBTEX));
}

@Disabled("Book entries can have the same ISBN due to different chapters. The Test fails as crossref identifies both entries as the same.")
/**
* Book entries can have the same ISBN due to different chapters. The Test fails as crossref identifies both entries as the same.
Copy link
Member

Choose a reason for hiding this comment

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

Use one space after * -- normal java doc style.

Modify the text. The test DOES NOT fail!

@AbdAlRahmanGad AbdAlRahmanGad marked this pull request as ready for review April 16, 2024 19:13
@koppor koppor enabled auto-merge April 16, 2024 21:04
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.

@koppor koppor added this pull request to the merge queue Apr 16, 2024
Merged via the queue into JabRef:main with commit d92c2d7 Apr 16, 2024
20 checks passed
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.

2 participants