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
…igrator to JSpecify annotations
…nstead of StudyYamlParser
…ith the rest of the codebase
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
… 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.
| 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.
jablib/src/main/java/org/jabref/logic/crawler/StudyYamlV1Migrator.java Outdated Show resolved Hide resolved
| | ||
| if (CURRENT_VERSION.equals(version)) { | ||
| // Already current version, read the file normally | ||
| try (InputStream fileInputStream = Files.newInputStream(studyYamlFile)) { |
There was a problem hiding this comment.
The try block covers the entire method, which is against best practices. It should cover as few statements as possible to minimize the scope of potential exceptions.
jablib/build.gradle.kts Outdated
| implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310") | ||
| // TODO: Somwewhere we get a warning: unknown enum constant Id.CLASS reason: class file for com.fasterxml.jackson.annotation.JsonTypeInfo$Id not found | ||
| // implementation("com.fasterxml.jackson.core:jackson-annotations:2.19.1") | ||
| implementation("com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.15.2") |
There was a problem hiding this comment.
Put the version in versions/build.gradle
Siedlerchr added the status: ready-for-review 
calixtus dismissed Siedlerchr’s stale review Version in versions/build.gradle is required.
calixtus left a comment
calixtus left a comment There was a problem hiding this comment.
Small issue about lucene open, but to move forward should be merged and done in a followup.
* upstream/main: Revert "Issue676 author last (JabRef#13625)" (JabRef#14123) Issue676 author last (JabRef#13625)
* upstream/main: New study.yml format (JabRef#13844) Separate issue linking workflows, make them run on `edited` (JabRef#14121)
| * Provides common functionality for version detection | ||
| * and handles the migration process | ||
| */ | ||
| |
| } | ||
| | ||
| @Test | ||
| void migrateEmptyStudySuccessfully() throws IOException { |
There was a problem hiding this comment.
Wrong approach, there should be two YAML files compared. Not input. YAML content and output: data model.
| We reverted this PR as not everything from the issue was done. We want to have a new intermediate release - and we cannot include some unfinished work. Sorry @turhantolgaunal that we are currently understaffed for this and you did not get any feedback in time. Maybe, another contributor builds on your work. The last two bullets points of the issue were not done  Moreover, tests were missing reading in the new YAML file structure. Tests should also go from strings to strings - not from strings to data model and vice versa. |
4 participants
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.
New format is:
version: 2.0
authors:
title: title
research-questions:
queries:
description: null
lucene: null
catalogue-specific: null
catalogues:
enabled: true
metadata:
notes: null
created-date: null
last-modified: null
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)