SLR : Improved study.yml format (v2) and terminology alignment#15215
Conversation
|
I've added the reason field directly into the StudyCatalog model. While it's not fully wired into the fxml yet in this specific commit I felt like having it in the data model would make sure that upcoming StudyYamlMigrator can handle the field mapping consistently from the start. |
This comment has been minimized.
This comment has been minimized.
6068de3 to
33ce1aa
Compare
This comment has been minimized.
This comment has been minimized.
33ce1aa to
41d3223
Compare
This comment has been minimized.
This comment has been minimized.
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
|
For submodule conflicts see https://devdocs.jabref.org/code-howtos/faq.html#submodules |
|
Refs #14170 |
|
I think, its OK for me to drop support for the old format. If anyone rasies their voice rearging this, we can work on this. One reason for this: the current code does not keep datastructrures for the v1 format, it just updates to the new one. |
|
Superficially, this PR should also include the other fixes outlined at #13844 (comment) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅🏷️ Commit: 2573254 Learn more about TestLens at testlens.app. |
|
Whts the status here? |
The Migrator is handling the v1 to v2 conversion automatically on read and the tests are passing as well. I do still have to extend StudyQuery to support catalogSpecific map for per-catalog native query overrides and also add the string to string tests that @koppor pointed out were missing from the original PR. I had finals going on at college so didn't really get much time to work on it in a while. I can possibly work on it next week. |
|
|
||
| // Observe changes to each item's enabledProperty so bindings re-evaluate when catalogs are toggled | ||
| private final ObservableList<StudyCatalogItem> databases = FXCollections.observableArrayList( | ||
| private final ObservableList<StudyCatalogItem> catalogs = FXCollections.observableArrayList( |
| Optional<StudyCatalog> match = studyCatalogs.stream().filter(catalog -> catalog.getName().equals(name)).findFirst(); | ||
| boolean enabled = match.map(StudyCatalog::isEnabled).orElse(false); | ||
| String reason = match.map(StudyCatalog::getReason).orElse(""); | ||
| return new StudyCatalogItem(name, enabled, reason); |
There was a problem hiding this comment.
Can this be simplified without Optional?
| public StudyCatalogItem(@NonNull String name, boolean enabled) { | ||
| this(name, enabled, ""); | ||
| } | ||
|
|
||
| public StudyCatalogItem(@NonNull String name, boolean enabled, @Nullable String reason) { | ||
| this.name = new SimpleStringProperty(name); | ||
| this.enabled = new SimpleBooleanProperty(enabled); | ||
| this.reason = new SimpleStringProperty(reason != null ? reason : ""); | ||
| } |
There was a problem hiding this comment.
If a method is overloaded for convenience, there should probably the explicit overloaded part not be nullable. Meaning: Fix in the constructor calls please.
| return reason.getValue(); | ||
| } | ||
|
|
||
| public void setReason(@Nullable String reason) { |
|
Has everything been addressed here? @faneeshh |
Yes. I just don't know what do with the 'heart' emoji review. #15215 (comment) |
subhramit
left a comment
There was a problem hiding this comment.
Getting this in as 3 people have reviewed.
Only open question may be regarding comment vs reason field, can be discussed in next developer call.
| @@ -0,0 +1,12 @@ | |||
| version: 2.0 | |||
| authors: | |||
| - Jab Ref | |||
There was a problem hiding this comment.
Use "real" fake names and not names never existing. This will get important if writing documentation.
Anna, Bob, ...
Thank you! I left minor comments.
According to #12631, it should be "reason" - the intention is to document on why a catalog was enabled or disabled. Maybe rename to "enabled-state-reason" to make it crystal clear? |
|
The follow-up issue is #14170 |
| // The user might add arbitrary content to the YAML | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class Study { | ||
| public static final String CURRENT_SCHEMA_VERSION = "2.0"; |
|
Please follow-up soon to use Semantic Versioning of the study.yml file - OR reason why SEMVER is not reasonable. I opted for SemVer, because it is proven and one does not need to evaluate other formats. |
Closes #12642 (hopefully)
This is basically a continuation of the work done in #13844. I'm starting with the foundational schema changed for the improved SLR support and I'm currently just gonna focus on getting the infrastructure right before diving into the actual fetcher logic.
Progress so far in the 1st Commit :
Terminology Alignment : Renamed StudyDatabase to StudyCatalog across the model and logic to match the UI and align with the domain language used in the repository
Schema Versioning : Added the version field to the Study model to support SEMVER (v2.0) which I feel is necessary for the migration
I'm attaching this rough sketch of how the data flow changes between the legacy format and the v2 format. I'm planning on creating a Migrator and string to string integration tests to ensure we don't break existing user studies
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)