Change format for study definition to yaml #7126

DominikVoigt · 2020-11-26T19:45:27Z

This PR replaces the previous use of a .bib file for the study definition file with a .yml (YAML) file.

An example study definition in YAML format:

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Signed-off-by: Dominik Voigt <[email protected]>

Siedlerchr · 2020-11-27T18:18:05Z

Houston, we have a problem in jlink:
Error: jdk.internal.org.objectweb.asm.MethodTooLargeException: Method too large: jdk/internal/module/SystemModules$all.moduleDescriptors ()[Ljava/lang/module/ModuleDescriptor;

We somehow need to reduce our modules...
https://bugs.openjdk.java.net/browse/JDK-8246197

tobiasdiez

I like this a lot! Thanks 👍

src/test/resources/org/jabref/logic/crawler/study.yml

src/main/java/org/jabref/model/study/QueryEntry.java

src/main/java/org/jabref/logic/crawler/LibraryEntryToFetcherConverter.java

src/main/java/org/jabref/model/study/Study.java

… StudyQuery.java Use hyphenated names in the yaml file Signed-off-by: Dominik Voigt <[email protected]>

DominikVoigt · 2020-11-29T17:26:52Z

Thanks for the feedback :) !
Regarding the removal of database and query entries from the study file I added an ADR why I believe that keeping them and adding the converter makes sense.
What do you think, did I miss any important arguments on either side?

…-with-yaml

Siedlerchr · 2020-11-29T18:01:52Z

I like the goal of this PR but we then have the problem which I stated earlier with the generated module

DominikVoigt · 2020-11-29T18:14:22Z

Yeah we probably cannot merge this until: https://bugs.openjdk.java.net/browse/JDK-8240567 is fixed

This is the bug the link you provided points to :)

tobiasdiez

So we cannot use the jackson library? What about the other ones? Is there a well-maintend one that is compatible with the module system?

tobiasdiez · 2020-11-29T20:05:05Z

docs/adr/0019-keep-study-as-a-dto.md

+
+### Keep study as DTO and use transformators
+
+* Good, because makes GUI implementation probably easier down the line


For the GUI, you probably need a wrapper around the study class anyway (and then it doesn't matter where the data is coming from).

So we cannot use the jackson library? What about the other ones? Is there a well-maintend one that is compatible with the module system?

Is this even related to jackson specifically? To me the amount of added modules seems to be problematic?

My guess is that jackson has a long module list which gives this error. But it could also be that all of our dependencies together are already at the borderline, and adding jackson just tipped us over the limit - this would be really bad indeed.

Should be easy to test though...

Yeah, I'll proceed with some testing now...

tobiasdiez · 2020-11-29T20:06:56Z

docs/adr/0019-keep-study-as-a-dto.md

+### Replace entries with instances
+
+* Good, because no need for database and query entries
+* Bad, because custom de-/serializers for fetchers and complex queries needed


That depends a bit on how complicated the custom serialization is. Since in the end you only need a map Fetcher <-> Name is needed, and such a conversion already exists.

The problem with this is that we need to keep the set of deactivated databases somewhere in memory to serialize them again.
In the future, we can replace this with records so I don't think this replacement is necessary now?
Regarding the complexity de-/serialization I'm not sure myself.

deactivated databases

That was actually something I was wondering about: why do you need to deactivate a fetcher (instead of simply removing it from the list)?

The idea behind this is to document that certain databases/libraries are actively not searched instead for example just disregarded/forgotten.
This is just another aspect to make the search traceable.

Ok, then we leave it as it's right now using custom Query/LibareryEntrys!

Btw, you don't need to write an ADR for every implementation detail. Here it would have be perfectly fine to just add a comment outlining the advantages you see.

I am aiming for creating a larger body of in-the-wirld ADRs to foster research on them. The example at hand can perfectly used to discuss the granularity of ADRs. - Thus, my wish is that the ADR is kept.

Siedlerchr · 2020-11-29T21:27:39Z

@tobiasdiez the problem is not the library itself, but the module itself. The jdk compiles all modules in a single class with a single method.
And now it happens that the class` method gets too large. This can happen when you have a certain amount of modules.
The linked jdk issue suggests a workaround, maybe that can work.
Otherwise we need to talk to someone to somehow bump the priority of the jdk issue

Signed-off-by: Dominik Voigt <[email protected]>

…-with-yaml

DominikVoigt · 2020-11-30T17:17:53Z

Okay so after some testing I suspect that this is an overall problem with the length/size of the module list, as in the test repository the same error can be observed with apache-commons being used instead of Jackson:
https://github.com/JabRef/jabref/pull/7146/checks?check_run_id=1475210612#step:11:2075

Apache Commons has no compile dependencies therefore the module list length cannot be a concern right?
In that case we could rule out the problem being connected to the Jackson module list length as mentioned by Tobias:

My guess is that jackson has a long module list which gives this error. But it could also be that all of our dependencies together are already at the borderline, and adding jackson just tipped us over the limit - this would be really bad indeed.

Which would mean, as Tobias mentions, that we face a larger problem?

koppor

Small comments on the ADRs

docs/adr/0018-use-Jackson-to-parse-study-yml.md

Co-authored-by: Oliver Kopp <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

…nikVoigt/jabref into replace-study-bib-with-yaml

Signed-off-by: Dominik Voigt <[email protected]>

…-with-yaml

Siedlerchr · 2020-12-06T21:03:29Z

try adding mainModule ='org.jabref'to the application blocj in build.gradle

Co-authored-by: Oliver Kopp <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

…nikVoigt/jabref into replace-study-bib-with-yaml

Signed-off-by: Dominik Voigt <[email protected]>

* upstream/master: Disable SLR in UI until #7126 is merged (#7179)

…-with-yaml

Siedlerchr · 2021-01-22T06:57:21Z

You could try again, we removed the graal dependency stuff, hope that helped

Siedlerchr · 2021-01-22T19:42:36Z

Yeah, removing graal helped

* upstream/master: Bump archunit-junit5-api from 0.15.0 to 0.16.0 (#7407) Bump classgraph from 4.8.98 to 4.8.102 (#7401) Bump archunit-junit5-engine from 0.15.0 to 0.16.0 (#7402) Bump mariadb-java-client from 2.7.1 to 2.7.2 (#7406) Bump org.beryx.jlink from 2.23.2 to 2.23.3 (#7400) Bump checkstyle from 8.39 to 8.40 (#7404) Ignore codecov status for automerge Fixes issue of Changing font size makes font size field too small (#7398) fix "Alt + keyboard shortcuts do not work" (#7379) Fixed invisible file path in the dark theme (#7396) Fix File Filter and some layout issues (#7385) Feature/implement complex queries (#7350) Change format for study definition to yaml (#7126) Fix handling of URL in file field (#7347) Fix expansion of bracketed expressions in RegExpBasedFileFinder (#7338)

DominikVoigt added 2 commits November 26, 2020 20:38

Change format for study definition to yaml and adapt tests

554f2f6

Signed-off-by: Dominik Voigt <[email protected]>

Add unversioned files

7728417

Signed-off-by: Dominik Voigt <[email protected]>

DominikVoigt force-pushed the replace-study-bib-with-yaml branch from a1027de to 7728417 Compare November 26, 2020 20:17

Remove unneeded enabled flags

8ed7442

Signed-off-by: Dominik Voigt <[email protected]>

tobiasdiez requested changes Nov 29, 2020

View reviewed changes

Rename LibraryEntry.java to StudyDatabase.java and QueryEntry.java to…

3b3c6c4

… StudyQuery.java Use hyphenated names in the yaml file Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into replace-study-bib…

4b699d6

…-with-yaml

tobiasdiez requested changes Nov 29, 2020

View reviewed changes

DominikVoigt added 2 commits November 30, 2020 16:16

Update ADR

0c99960

Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into replace-study-bib…

e0dd2e6

…-with-yaml

koppor reviewed Nov 30, 2020

View reviewed changes

DominikVoigt and others added 9 commits December 1, 2020 08:00

Update docs/adr/0018-use-Jackson-to-parse-study-yml.md

1fdd202

Co-authored-by: Oliver Kopp <[email protected]>

Update docs/adr/0018-use-Jackson-to-parse-study-yml.md

a0e1c07

Co-authored-by: Oliver Kopp <[email protected]>

Apply suggestions from code review

76bc7d0

Co-authored-by: Oliver Kopp <[email protected]>

Fix crawler test

9e4f1a0

Signed-off-by: Dominik Voigt <[email protected]>

Merge branch 'replace-study-bib-with-yaml' of https://github.com/Domi…

cb449f1

…nikVoigt/jabref into replace-study-bib-with-yaml

Fix crawler test

0e7e7a6

Signed-off-by: Dominik Voigt <[email protected]>

Add some links to ADR 0018

0d861c8

Signed-off-by: Dominik Voigt <[email protected]>

Add indent for readability

fcabebb

Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into replace-study-bib…

20a80e7

…-with-yaml

DominikVoigt and others added 3 commits December 6, 2020 23:24

Update docs/adr/0018-use-Jackson-to-parse-study-yml.md

c0f734f

Co-authored-by: Oliver Kopp <[email protected]>

Add main module to gradle build

8c2320b

Signed-off-by: Dominik Voigt <[email protected]>

Merge branch 'replace-study-bib-with-yaml' of https://github.com/Domi…

832cdb6

…nikVoigt/jabref into replace-study-bib-with-yaml

Remove duplicate links

37bfa52

Signed-off-by: Dominik Voigt <[email protected]>

koppor mentioned this pull request Dec 7, 2020

Remove menu for study bib #7168

Closed

DominikVoigt added a commit that referenced this pull request Dec 12, 2020

Disable SLR in UI until #7126 is merged

605b179

DominikVoigt mentioned this pull request Dec 12, 2020

Disable SLR in UI until #7126 is merged #7179

Merged

5 tasks

tobiasdiez pushed a commit that referenced this pull request Dec 12, 2020

Disable SLR in UI until #7126 is merged (#7179)

86e2c82

Siedlerchr added a commit that referenced this pull request Dec 13, 2020

Merge remote-tracking branch 'upstream/master' into sharedDbInsert

c6d01b3

* upstream/master: Disable SLR in UI until #7126 is merged (#7179)

Merge remote-tracking branch 'upstream/master' into replace-study-bib…

86a25b4

…-with-yaml

koppor mentioned this pull request Jan 4, 2021

Pure Java implementation michel-kraemer/citeproc-java#92

Closed

koppor mentioned this pull request Jan 18, 2021

[WIP/Testing] Test Module Issue #7146

Closed

5 tasks

DominikVoigt added 2 commits January 22, 2021 16:11

Merge upstream/master into current branch.

a92215d

Fix checkstyle issue

4736a34

correct casing

4a564b3

koppor approved these changes Jan 26, 2021

View reviewed changes

koppor merged commit 7117b61 into JabRef:master Jan 26, 2021

DominikVoigt deleted the replace-study-bib-with-yaml branch February 11, 2021 15:35

tobiasdiez mentioned this pull request Dec 5, 2021

Integration with JabRef online #7832

Draft

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change format for study definition to yaml #7126

Change format for study definition to yaml #7126

DominikVoigt commented Nov 26, 2020

Siedlerchr commented Nov 27, 2020 •

edited

Loading

tobiasdiez left a comment

DominikVoigt commented Nov 29, 2020

Siedlerchr commented Nov 29, 2020

DominikVoigt commented Nov 29, 2020 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Nov 29, 2020

DominikVoigt Nov 29, 2020

tobiasdiez Nov 30, 2020

DominikVoigt Nov 30, 2020 •

edited

Loading

tobiasdiez Nov 29, 2020

DominikVoigt Nov 29, 2020

tobiasdiez Nov 30, 2020

DominikVoigt Nov 30, 2020

tobiasdiez Nov 30, 2020

koppor Nov 30, 2020

Siedlerchr commented Nov 29, 2020

DominikVoigt commented Nov 30, 2020 •

edited

Loading

koppor left a comment

Siedlerchr commented Dec 6, 2020 •

edited

Loading

Siedlerchr commented Jan 22, 2021

Siedlerchr commented Jan 22, 2021


		### Keep study as DTO and use transformators

		* Good, because makes GUI implementation probably easier down the line

Change format for study definition to yaml #7126

Change format for study definition to yaml #7126

Conversation

DominikVoigt commented Nov 26, 2020

Siedlerchr commented Nov 27, 2020 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

DominikVoigt commented Nov 29, 2020

Siedlerchr commented Nov 29, 2020

DominikVoigt commented Nov 29, 2020 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominikVoigt Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Nov 29, 2020

DominikVoigt commented Nov 30, 2020 • edited Loading

koppor left a comment

Choose a reason for hiding this comment

Siedlerchr commented Dec 6, 2020 • edited Loading

Siedlerchr commented Jan 22, 2021

Siedlerchr commented Jan 22, 2021

Siedlerchr commented Nov 27, 2020 •

edited

Loading

DominikVoigt commented Nov 29, 2020 •

edited

Loading

DominikVoigt Nov 30, 2020 •

edited

Loading

DominikVoigt commented Nov 30, 2020 •

edited

Loading

Siedlerchr commented Dec 6, 2020 •

edited

Loading