New study.yml format #13844
New study.yml format #13844
Conversation
turhantolgaunal
changed the title
…ns from the current study.yml version to the new one
…n and parsing of study.yml if needed
…r to avoid deadlock
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
|
|
||
| if (CURRENT_VERSION.equals(version)) { | ||
| // Already current version, read the file normally | ||
| ObjectMapper yamlMapper = new ObjectMapper(new YAMLFactory()); |
There was a problem hiding this comment.
The ObjectMapper instance is created multiple times in the code. It should be extracted to a static final field to improve performance and reduce object creation.
…igrator to JSpecify annotations
…nstead of StudyYamlParser
…ith the rest of the codebase
| @@ -196,10 +196,10 @@ public List<String> getSearchQueryStrings() { | |||
| * @return List of BibEntries of type Library | |||
| * @throws IllegalArgumentException If a transformation from Library entry to LibraryDefinition fails | |||
| */ | |||
| public List<StudyDatabase> getActiveLibraryEntries() throws IllegalArgumentException { | |||
| return study.getDatabases() | |||
| public List<StudyCatalog> getActiveLibraryEntries() throws IllegalArgumentException { | |||
There was a problem hiding this comment.
The method getActiveLibraryEntries should not return null. It should return an empty list using List.of() if there are no active library entries.
| * Generate notes for the migration | ||
| */ | ||
| private String generateMigrationNotes(V1StudyFormat oldStudy) { | ||
| StringBuilder notes = new StringBuilder(); |
There was a problem hiding this comment.
StringBuilder is used for constructing strings, but StringJoiner should be used instead if possible for better readability and maintainability.
| return "1.0"; | ||
| } | ||
|
|
||
| private Study convertToCurrentFormat(V1StudyFormat oldStudy, Path studyYamlFile) { |
There was a problem hiding this comment.
The method convertToCurrentFormat should use Optional instead of returning null to handle cases where a Study object might not be created.
| return notes.toString().trim(); | ||
| } | ||
|
|
||
| private StudyQuery convertQuery(Object queryObj) { |
There was a problem hiding this comment.
The method convertQuery should not return null. It should use Optional to handle cases where a StudyQuery object might not be created.
| List<StudyCatalog> catalogs = migratedStudy.getCatalogs(); | ||
| assertEquals(2, catalogs.size()); | ||
| assertEquals("IEEE", catalogs.get(0).getName()); | ||
| assertTrue(catalogs.get(0).isEnabled()); // Should default to true |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
| assertEquals("IEEE", catalogs.get(0).getName()); | ||
| assertTrue(catalogs.get(0).isEnabled()); // Should default to true | ||
| assertEquals("ACM", catalogs.get(1).getName()); | ||
| assertFalse(catalogs.get(1).isEnabled()); // Should preserve false |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
| StudyMetadata metadata = migratedStudy.getMetadata(); | ||
|
|
||
| // Check migration notes | ||
| assertTrue(metadata.getNotes().contains("Migrated from v1.0 format")); |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
|
|
||
| // Check migration notes | ||
| assertTrue(metadata.getNotes().contains("Migrated from v1.0 format")); | ||
| assertTrue(metadata.getNotes().contains("1 search queries")); |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
| // Check migration notes | ||
| assertTrue(metadata.getNotes().contains("Migrated from v1.0 format")); | ||
| assertTrue(metadata.getNotes().contains("1 search queries")); | ||
| assertTrue(metadata.getNotes().contains("1 active databases")); |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
| Study migratedStudy = migrator.migrate(studyFile); | ||
|
|
||
| String notes = migratedStudy.getMetadata().getNotes(); | ||
| assertTrue(notes.contains("Migrated from v1.0 format")); |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
|
|
||
| String notes = migratedStudy.getMetadata().getNotes(); | ||
| assertTrue(notes.contains("Migrated from v1.0 format")); | ||
| assertTrue(notes.contains("3 search queries")); |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
| String notes = migratedStudy.getMetadata().getNotes(); | ||
| assertTrue(notes.contains("Migrated from v1.0 format")); | ||
| assertTrue(notes.contains("3 search queries")); | ||
| assertTrue(notes.contains("2 active databases")); |
There was a problem hiding this comment.
The assertion checks a boolean condition instead of asserting the expected value directly, which is less clear and can lead to less informative test failures.
… from getCatalogSpecific form StudyQuery
| @@ -8,7 +8,7 @@ | |||
| import org.jabref.logic.importer.ImportFormatPreferences; | |||
| import org.jabref.logic.importer.ImporterPreferences; | |||
| import org.jabref.model.study.Study; | |||
| import org.jabref.model.study.StudyDatabase; | |||
| import org.jabref.model.study.StudyCatalog; | |||
There was a problem hiding this comment.
The import statement for StudyCatalog is correct, but the patch does not address the need to use JavaFX ObservableLists for better practice in managing lists in JavaFX applications.
| @@ -92,14 +92,14 @@ void studyConstructorFillsDatabasesCorrectly(@TempDir Path tempDir) { | |||
| } | |||
|
|
|||
| private ManageStudyDefinitionViewModel getManageStudyDefinitionViewModel(Path tempDir) { | |||
| List<StudyDatabase> databases = List.of( | |||
| new StudyDatabase("ACM Portal", true)); | |||
| List<StudyCatalog> catalogs = List.of( | |||
There was a problem hiding this comment.
The code uses List.of() which is correct, but it does not utilize JavaFX ObservableLists, which is considered best practice for managing lists in JavaFX applications.
|
|
||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.nio.file.Files; |
There was a problem hiding this comment.
The code uses java.nio.file.Files for file operations. Consider using modern Java best practices like Path.of() instead of Paths.get() for improved readability and maintainability.
| this.parser = new StudyYamlParser(); | ||
| } | ||
|
|
||
| public StudyYamlService(StudyYamlParser parser) { |
There was a problem hiding this comment.
The constructor allows null to be passed to the parser parameter, which can lead to NullPointerExceptions. Consider using java.util.Optional or JSpecify annotations to handle nullability.
| public Map<String, String> getCatalogSpecific() { | ||
| return catalogSpecific != null ? Collections.unmodifiableMap(catalogSpecific) : null; | ||
| } |
There was a problem hiding this comment.
The method getCatalogSpecific() returns null if catalogSpecific is null. It should return an Optional instead of null to follow modern Java best practices.
| StudyYamlParser studyYAMLParser = new StudyYamlParser(); | ||
| studyYAMLParser.writeStudyYamlFile(newStudy, studyRepositoryRoot.resolve(StudyRepository.STUDY_DEFINITION_FILE_NAME)); | ||
| StudyYamlService studyYamlService = new StudyYamlService(); | ||
| studyYamlService.writeStudyYamlFile(newStudy, studyRepositoryRoot.resolve(StudyRepository.STUDY_DEFINITION_FILE_NAME)); |
There was a problem hiding this comment.
The method writeStudyYamlFile should ensure it does not return null. If it does, it should use java.util.Optional to handle potential null values.
|
Your code currently does not meet JabRef's code guidelines. IntelliJ auto format covers some cases. There seem to be issues with your code style and autoformat configuration. Please reformat your code (Ctrl+Alt+L) and commit, then push. In special cases, consider using |
Closes #12642
The study.yml is refactored to a new format to implement SEMVER versioning, variable 'databases' is renamed to 'catalogues' for UI consistency.
In the course of working on this draft pull request the query structure is planned to be enhanced to support multiple query formats including catalog-specific variations.
Steps to test
The study.yml file created when a new slr is started should be in a new format and slr search should function as intended.
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)