Feature store journal lists in seperate directory #10824
Conversation
…iles, refactoring
| @@ -15,6 +15,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv | |||
| - We added a fetcher for [ISIDORE](https://isidore.science/), simply paste in the link into the text field or the last 6 digits in the link that identify that paper. [#10423](https://github.com/JabRef/jabref/issues/10423) | |||
| - When importing entries form the "Citation relations" tab, the field [cites](https://docs.jabref.org/advanced/entryeditor/entrylinks) is now filled according to the relationship between the entries. [#10572](https://github.com/JabRef/jabref/pull/10752) | |||
| - We added a new group icon column to the main table showing the icons of the entry's groups. [#10801](https://github.com/JabRef/jabref/pull/10801) | |||
| - We added journal abbreviations directory in jabref data directory with automatic processing. [#10557](https://github.com/JabRef/jabref/issues/10557) | |||
There was a problem hiding this comment.
As this feature is not really facing the frontend ui, I tend not to put this to the changelog. Are there other opinions?
There was a problem hiding this comment.
This is only a part of the issue. The issue also lists "custom.csv" which should be mentioned.
| @@ -70,6 +70,6 @@ public static JournalAbbreviationRepository loadRepository(JournalAbbreviationPr | |||
| } | |||
|
|
|||
| public static JournalAbbreviationRepository loadBuiltInRepository() { | |||
| return loadRepository(new JournalAbbreviationPreferences(Collections.emptyList(), true)); | |||
| return loadRepository(new JournalAbbreviationPreferences(Collections.emptyList(), true, null)); | |||
There was a problem hiding this comment.
Passing null to a constructor is bad style. There should be a constructor without the additional argument...
| this.externalJournalLists = FXCollections.observableArrayList(externalJournalLists); | ||
| this.useFJournalField = new SimpleBooleanProperty(useFJournalField); | ||
| this.journalAbbreviationsDirectory.setValue(journalAbbreviationsDirectory); |
There was a problem hiding this comment.
Such a method should not accept null
| if (path.getParent() == null) { | ||
| return; | ||
| } | ||
| String mvFileName = path.getFileName().toString().split("\\.")[0] + ".mv"; |
There was a problem hiding this comment.
Use javas basename feature, not string magic
| @@ -468,6 +468,7 @@ JabRef\ requests\ recommendations\ from\ Mr.\ DLib,\ which\ is\ an\ external\ se | |||
| JabRef\ Version\ (Required\ to\ ensure\ backwards\ compatibility\ with\ Mr.\ DLib's\ Web\ Service)=JabRef Version (Required to ensure backwards compatibility with Mr. DLib's Web Service) | |||
|
|
|||
| Journal\ abbreviations=Journal abbreviations | |||
| Journal\ abbreviations\ directory\:=Journal abbreviations directory: | |||
There was a problem hiding this comment.
Remove the : at the end. We aim to get rid of these.
| cleanupDir(workDir); | ||
| } | ||
|
|
||
| public void cleanupDir(File dir) { |
There was a problem hiding this comment.
Use @tempdir instead of manual test file management
| public class JournalAbbreviationStorerTest { | ||
|
|
||
| @Test | ||
| public void shouldStoreMVFile() throws IOException { |
| @@ -313,7 +327,8 @@ public void saveJournalAbbreviationFiles() { | |||
| journalFiles.forEach(file -> { | |||
| try { | |||
| file.writeOrCreate(); | |||
| } catch (IOException e) { | |||
| } catch ( | |||
| IOException e) { | |||
There was a problem hiding this comment.
Our IntellIj formatter is broken here somehow..please remove the newline manually.
Siedlerchr
added
the
status: changes required
|
@MathewKnee Do you have time to work on this? I would be nice if the smaller issues were resolved so that the next review round can start on "clean code". |
|
Closing this PR due to inactivity 💤 |
Closes #10557
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)