Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change format for study definition to yaml #7126

Merged
merged 24 commits into from
Jan 26, 2021

Conversation

DominikVoigt
Copy link
Contributor

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:
image

  • 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
Copy link
Member

Siedlerchr commented Nov 27, 2020

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

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot! Thanks 👍

src/test/resources/org/jabref/logic/crawler/study.yml Outdated Show resolved Hide resolved
src/test/resources/org/jabref/logic/crawler/study.yml Outdated Show resolved Hide resolved
src/main/java/org/jabref/model/study/QueryEntry.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/model/study/Study.java Outdated Show resolved Hide resolved
… StudyQuery.java

Use hyphenated names in the yaml file

Signed-off-by: Dominik Voigt <[email protected]>
@DominikVoigt
Copy link
Contributor Author

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?

@Siedlerchr
Copy link
Member

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

@DominikVoigt
Copy link
Contributor Author

DominikVoigt commented Nov 29, 2020

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 :)

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


### Keep study as DTO and use transformators

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@DominikVoigt DominikVoigt Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

### Replace entries with instances

* Good, because no need for database and query entries
* Bad, because custom de-/serializers for fetchers and complex queries needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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

@DominikVoigt
Copy link
Contributor Author

DominikVoigt commented Nov 30, 2020

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?

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments on the ADRs

docs/adr/0018-use-Jackson-to-parse-study-yml.md Outdated Show resolved Hide resolved
docs/adr/0018-use-Jackson-to-parse-study-yml.md Outdated Show resolved Hide resolved
docs/adr/0018-use-Jackson-to-parse-study-yml.md Outdated Show resolved Hide resolved
docs/adr/0018-use-Jackson-to-parse-study-yml.md Outdated Show resolved Hide resolved
@Siedlerchr
Copy link
Member

Siedlerchr commented Dec 6, 2020

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

Signed-off-by: Dominik Voigt <[email protected]>
@koppor koppor mentioned this pull request Dec 7, 2020
DominikVoigt added a commit that referenced this pull request Dec 12, 2020
tobiasdiez pushed a commit that referenced this pull request Dec 12, 2020
Siedlerchr added a commit that referenced this pull request Dec 13, 2020
* upstream/master:
  Disable SLR in UI until #7126 is merged (#7179)
@Siedlerchr
Copy link
Member

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

@Siedlerchr
Copy link
Member

Yeah, removing graal helped

@koppor koppor merged commit 7117b61 into JabRef:master Jan 26, 2021
Siedlerchr added a commit that referenced this pull request Feb 1, 2021
* 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 DominikVoigt deleted the replace-study-bib-with-yaml branch February 11, 2021 15:35
@tobiasdiez tobiasdiez mentioned this pull request Dec 5, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants