-
-
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
Added default journal abbreviation setting #12105
base: main
Are you sure you want to change the base?
Added default journal abbreviation setting #12105
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
if (entry.hasField(StandardField.JOURNALTITLE)) { | ||
String oldJournalTitle = entry.getField(StandardField.JOURNALTITLE).orElse(null); |
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.
Try to use ...ifPresent(content -> {...}
.
The .orElse(null)
is a no-go. - Also just .get()
directly after the hasField
is an indication for
entry.getField(StandardField.JOURNALTITLE).ifPresent(content -> {
...
}
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, you should use the latexFree variant (org.jabref.model.entry.BibEntry#getFieldLatexFree).
Store the two fields in a List and iterate through the list - to have no code duplilcation of the handling part.
|
||
UndoableAbbreviator undoableAbbreviator = new UndoableAbbreviator(repository, AbbreviationType.DEFAULT, false); | ||
|
||
// Abbreviate journal and journaltitle fields |
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.
No need for that comment - the code saxs it.
public List<FieldChange> cleanup(BibEntry entry) { | ||
List<FieldChange> changes = new ArrayList<>(); | ||
|
||
// Skip if preferences is disabled |
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.
Stands for itself. remove comemt.
// Skip if preferences is disabled | ||
boolean shouldAutoAbbreviateJournals = JabRefGuiPreferences.getInstance().getImporterPreferences().shouldAutoAbbreviateJournals(); | ||
if (!shouldAutoAbbreviateJournals) { | ||
return changes; |
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.
Return List.of()
and initialize changes
later.
boolean shouldAutoAbbreviateJournals = JabRefGuiPreferences.getInstance().getImporterPreferences().shouldAutoAbbreviateJournals(); | ||
if (!shouldAutoAbbreviateJournals) { |
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.
Merge these two lines.
Please think about "test-driven development". If there are "only" implementation-style comments (as they currently are), the tests should not change. -- I know that test refactoring is hard on larger code changes; but at least you have test data available. This can be done regardless of the concrete implementation. Thus, always work on tests while implementing something. |
Changes Made
Added a UI option in the Web Search tab of preferences to allow journals to be automatically abbreviated when fetching from the web.
org.jabref.logic.importer.ParserFetcher#doPostCleanup
implements the newly addedAbbreviateJournalCleanup
task by default, which itself uses theUndoableAbbreviator
. Overriding implementations ofParserFetcher#doPostCleanup
were also changed to add the cleanup job.Current Issues
The
AbbreviateJournalCleanup
uses a deprecated methodJabRefGuiPreferences.getInstance()
to obtainImporterPreferences
. However, this implementation was done to avoid changing the constructor arguments for each fetcher (e.g.CrossRef() → CrossRef(ImporterPreferences importerPreferences
). One possible alternative is addingImporterPreferences
toImportFormatPreferences
, as this is already passed to many fetchers. However, this does not apply to all, and would seemingly require changes to a large amount of method signatures (i.e. functions that call fetchers, as innew CrossRef()
. Suggestions for an improved implementation are welcome.Changes will also likely need to merged with the refactoring of
UndoableAbbreviator
, outlined in #12101 and #11791.Other Notes
Tests will need to be created upon implementation methods being approved.
Due to being a draft pull request, note that
CHANGELOG.md
and relevant documentation has not yet been modified. Will be completed upon approval.Closes #11305
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)