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.

Sign up for GitHub

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

Jump to bottom

Adjust zbmath fetcher #7298

Merged
merged 7 commits into from
Jan 7, 2021
Merged

Adjust zbmath fetcher #7298

merged 7 commits into from
Jan 7, 2021

Conversation

ibe-314
Copy link
Contributor

@ibe-314 ibe-314 commented Jan 5, 2021 •
edited by Siedlerchr
Loading

I implemented an IdBasedParserFetcher for the zbMATH fetcher [#7202].

fixes #7202

zbMATHidfetcher

Running tests with gradle does not work locally on my computer, thus I create a draft pull request to run the tests. As I only changed the zbMATH fetcher, only zbMATHTest.java should be affected

  • [ x] Change in CHANGELOG.md described (if applicable)
  • [ x] Tests created for changes (if applicable)
  • [ x] Manually tested changed features in running JabRef (always required)
  • [ x] Screenshots added in PR description (for UI changes)
  • [ x] 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. -> Will create a pull request to update documentation.

@ibe-314 ibe-314 mentioned this pull request Jan 5, 2021
Closed
@Siedlerchr
Copy link
Member

Looks already good so far, I think the only difference is now that the search returns more than one entry:

org.jabref.logic.importer.fetcher.ZbMATHTest

Test searchByQueryFindsEntry() FAILED

@ibe-314
Copy link
Contributor Author

ibe-314 commented Jan 5, 2021 •
edited
Loading

Looks already good so far, I think the only difference is now that the search returns more than one entry:

org.jabref.logic.importer.fetcher.ZbMATHTest

Test searchByQueryFindsEntry() FAILED

I think this failure is not due to my changes. The searchByQueryFindsEntry() tests the zbmath SearchBasedParserFetcher implementation that I have not changed. This test was disabled on CIServer and I enabled it because no subscribtion to zbMATH is needed anymore. I think the FAILED test was just not visible before.

The problem here is that the search query an:0507.57010 is transformed into "0507.57010" before it is passed to the SearchBasedParserFetcher.
I added a print statement in the function getURLforQuery in zbMATH Fetcher https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/logic/importer/fetcher/ZbMATH.java. Then I started jabref using gradle and tested the web search using the gui. My input is an:0507.57010. However, the function getURLforQuery does not recive an:0507.57010 as its query parameter but "0507.57010" . So somewhere the input query is transformed. I also tested other structured queries for zbmath (e.g. au:Berge ti:"Graph Theory" is transformed to "Graph Theory" "Berge" ) and it always transforms the query, which is not so good in this case as no structured query is possible. Is there a way to directly pass the user input string to the query parameter at getURLforQuery?

@tobiasdiez
Copy link
Member

tobiasdiez commented Jan 6, 2021 •
edited
Loading

I think this problem comes from the recent improvements to support complex queries. The an: query part is not supported and thus removed. @DominikVoigt can probably say more about this.

The test should be changed either way to search by say using the title and author, to better reflect how a user would work with the fetcher.

These things are not really related to this PR though, so they can also be done in a follow-up PR!

@ibe-314
Copy link
Contributor Author

ibe-314 commented Jan 6, 2021

Thank you for the information.
I did not know about the new search query. Is it described anywhere? I tried the query author:"Claude Berge" and it returned also documents that are not authored by Claude Berge.

Back to this PR: I added a test called searchByIdFindsEntry() at org.jabref.logic.importer.fetcher.ZbMATHTest to test the implementation of the IdBasedParserFetcher. When I look at the logs of Fetcher Test, then this test does not appear, thus I guess that it was successful.

tobiasdiez
tobiasdiez requested changes Jan 6, 2021
View reviewed changes
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 agree, this is not really related to this PR.

Codewise this looks good to me. Please improve the changelog entry a bit, then this is ready to go from my side. Thanks again!

CHANGELOG.md Outdated
@@ -11,6 +11,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve

### Added

- We implemented an IdBasedParserFetcher for zbmath.
Copy link
Member

Choose a reason for hiding this comment

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

The changelog is mostly for our users. Could you thus provide a more "understandable" explanation. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the changelog.

@tobiasdiez tobiasdiez marked this pull request as ready for review January 6, 2021 21:35
@tobiasdiez tobiasdiez added status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Jan 6, 2021
@Siedlerchr Siedlerchr removed the status: changes required Pull requests that are not yet complete label Jan 7, 2021
@Siedlerchr Siedlerchr merged commit 129abaa into JabRef:master Jan 7, 2021
@Siedlerchr
Copy link
Member

Thanks a lot for your contribution! That will be very useful for users in that field of research.

Siedlerchr added a commit that referenced this pull request Jan 7, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into importerbackgroun…
179c349 
…dtask

* upstream/master:
  Adjust zbmath fetcher (#7298)
  Add "acm-siggraph.csl" required by CitationStyle.java
  Added Keyboard shortcuts (clear/set read status) (#7302)
  Add special fields ADR (#7300)
  Overwrite local copies
  Squashed 'buildres/csl/csl-locales/' content from commit ecb8e70233
  Squashed 'buildres/csl/csl-styles/' content from commit 737ffa1
  Adapt workflow and build.gradle
  Move CSL to buildres/csl to speedup "processResources" during development
Siedlerchr added a commit that referenced this pull request Jan 10, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into test-module-issue
4508044 
* upstream/master: (34 commits)
  Fixed exception about missing custom css file (#7292)
  Update the templates for opening a new issue (#7321)
  Entitlements file Mac (#7317)
  Make CONTRIBUTING.md much shorter. Move long text to docs/contributing.md (#7293)
  Include Github-optimized screenshot into repository (#7318)
  Remove obsolete registry patch file (#7316)
  Fix AUTHORS
  GitBook: [master] one page modified
  Remove broken Sonarqube integration (#7315)
  GitBook: [master] 5 pages and 32 assets modified
  docs: update license year (#7314)
  Add javafx version number + update javafx (#7312)
  Add missing authors
  Adjust zbmath fetcher (#7298)
  Add "acm-siggraph.csl" required by CitationStyle.java
  Added Keyboard shortcuts (clear/set read status) (#7302)
  Add special fields ADR (#7300)
  Overwrite local copies
  Squashed 'buildres/csl/csl-locales/' content from commit ecb8e70233
  Squashed 'buildres/csl/csl-styles/' content from commit 737ffa1
  ...
@ibe-314 ibe-314 deleted the adjust-zbmath-fetcher branch February 8, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez requested changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support to add an entry from zbmath.org via its zbMATH id
3 participants
@ibe-314 @Siedlerchr @tobiasdiez