-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 EprintCleanup to handle institution, version, and EID fields #11627
Conversation
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.
Small comment.
Moreover, please add the link to the documentation of the identifier, Tobias provided. That knowledge should be persistet in the code!
@Test | ||
void cleanupEntryWithVersionAndInstitutionAndEid() { | ||
BibEntry input = new BibEntry() | ||
.withField(StandardField.NOTE, "arXiv: 1503.05173") | ||
.withField(StandardField.VERSION, "1") | ||
.withField(StandardField.INSTITUTION, "arXiv") | ||
.withField(StandardField.EID, "arXiv:1503.05173"); | ||
|
||
BibEntry expected = new BibEntry() | ||
.withField(StandardField.EPRINT, "1503.05173v1") | ||
.withField(StandardField.EPRINTTYPE, "arxiv"); | ||
|
||
EprintCleanup cleanup = new EprintCleanup(); | ||
cleanup.cleanup(input); | ||
|
||
assertEquals(expected, input); | ||
} |
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.
Please add another test with INSTITUTION
tbd
. That field value should be kept IMHO. --> JabRef should not destory data.
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.
@koppor I have made the suggested changes and pushed it to this PR , please review.
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.
I think, I was not clear with another
This means: A second test case. This means: Keep the original test as is.
I did not also not "unzip" my comment, sorry!
This will require changes in the code. The code needs to check if the institution is arXiv
. Then, the field can be cleared. Otherwise, the field needs to be kept.
@Test | ||
void cleanupEntryWithVersionAndInstitutionAndEid() { | ||
BibEntry input = new BibEntry() | ||
.withField(StandardField.NOTE, "arXiv: 1503.05173") | ||
.withField(StandardField.VERSION, "1") | ||
.withField(StandardField.INSTITUTION, "arXiv") | ||
.withField(StandardField.EID, "arXiv:1503.05173"); | ||
|
||
BibEntry expected = new BibEntry() | ||
.withField(StandardField.EPRINT, "1503.05173v1") | ||
.withField(StandardField.EPRINTTYPE, "arxiv"); | ||
|
||
EprintCleanup cleanup = new EprintCleanup(); | ||
cleanup.cleanup(input); | ||
|
||
assertEquals(expected, input); | ||
} |
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.
I think, I was not clear with another
This means: A second test case. This means: Keep the original test as is.
I did not also not "unzip" my comment, sorry!
This will require changes in the code. The code needs to check if the institution is arXiv
. Then, the field can be cleared. Otherwise, the field needs to be kept.
BibEntry expected = new BibEntry() | ||
.withField(StandardField.EPRINT, "1503.05173v1") | ||
.withField(StandardField.EPRINTTYPE, "arxiv") | ||
.withField(StandardField.INSTITUTION, "tbd"); |
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 use other-institution
as example for the new test.
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.
@koppor I have done the necessary changes, updated the EprintCleanup class to clear the institution field if it is "arXiv" else keep it otherInstitution as it is.
@18bce133 Hey, how is it going? Are you still working on this PR? |
Hey @subhramit ! I'm still on the PR. I've been quite busy lately with some interviews lined up, which slowed me down a bit. I'll make sure to wrap up this PR in the next few days. Thanks for checking in! |
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.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
CHANGELOG.md was not modified. I add the entry by myself to get this merged. |
What
This PR refactors the EprintCleanup class to handle additional fields: INSTITUTION, VERSION, and EID. It also includes new tests to ensure the correct functionality of these changes.
Why
The changes are necessary to improve the handling of e-print entries by including more comprehensive cleanup logic. This ensures that entries with INSTITUTION, VERSION, and EID fields are properly normalized and cleaned up.
Changes
Issue
Closes #11306
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)