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 FieldComparator vs. EntryComparator #497

Open
koppor opened this issue May 10, 2021 · 3 comments
Open

Fix FieldComparator vs. EntryComparator #497

koppor opened this issue May 10, 2021 · 3 comments

Comments

@koppor
Copy link
Owner

koppor commented May 10, 2021

The EntryComparator does not call the FieldComparator.

Why?

Should be fixed. - At least JavaDoc should added referencing each other and explaining the differences.

Refs JabRef#7708

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Collaborator

k3KAW8Pnf7mkmdSMPHz27 commented May 12, 2021

I'll view this as a bit of a mid-length project. Preliminarily I am looking at

  1. BibtexStringComparator
  2. EntryComparator (break ties with Id)
  3. FieldComparator
  4. FieldComparatorStack (doesn't break ties with Id) -> replace with Comparator.andThen or .reduce(Comparator::thenComparing) -> removed
  5. CrossRefEntryComparator
  6. NumericFieldComparator -> might make a change to its logic, will add test cases if I do
  7. IdComparator
  8. RankingFieldComparator
  9. SpecialFieldComparator
  10. StringLengthComparator -> two uses with Comparator.comparingInt(String::length).reversed()) -> removed

and it seems that EntryComparator and FieldComparatorStack serve the same purpose.

Most of them are essentially unchanged in the last 8 years (i.e., they are pre-GitHub) so I'll take a look at modernizing the ones I believe need it. The changes should lead to a lot less code that is more readable (❤️ Java 8+) so I'd guess it is a one PR change.

Todo

  1. Verify that I can use reduce

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Collaborator

Should the PR go here or in the JabRef/jabref?

@koppor
Copy link
Owner Author

koppor commented Mar 15, 2023

I'd guess it is a one PR change.

I fully agree.

Therefore I woud open the PR against JabRef/jabref. Here, we collect PRs, which are suspended or take a veeeeeery long time to finish. This issue here is more a short-term one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants