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

Jstor Fetcher #6992

Merged
merged 8 commits into from
Oct 14, 2020
Merged

Jstor Fetcher #6992

merged 8 commits into from
Oct 14, 2020

Conversation

joethei
Copy link
Contributor

@joethei joethei commented Oct 8, 2020 •
edited
Loading

added Fetcher for jstor.org

Fixes #6627

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

Siedlerchr
Siedlerchr requested changes Oct 8, 2020
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Welcome to JabRef! Thanks for your contribution! That's a nice addition. 👍 Overall the code looks good so far, just a few improvements
It would be cool if you could check if it's possible to extend the fetcher so it's a FullTextFetcher and downloads the pdf?

src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java Outdated Show resolved Hide resolved
src/test/java/org/jabref/logic/importer/fetcher/JstorFetcherTest.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/logic/importer/WebFetchers.java Outdated Show resolved Hide resolved
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 8, 2020
@joethei joethei requested a review from Siedlerchr October 9, 2020 09:35
Siedlerchr
Siedlerchr approved these changes Oct 9, 2020
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

LGTM now. Please fix the remaining check style issues.
For external contributes we require that at least two JabRef devs do a code review before it's merged.

@koppor
Copy link
Member

koppor commented Oct 12, 2020

Thank you for the PR and welcome to the open source world of JabRef. Maybe you already registered for hacktoberfest. This PR will definitively count towards Hacktoberfest and we are looking for more.

I will go through your code soon. Meanwhile, may I ask to fix the checktyle issues?

Error: eckstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java:84:9: 'if' is not followed by whitespace. [WhitespaceAround]
Error: eckstyle] [ERROR] /home/runner/work/jabref/jabref/src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java:93:9: 'if' is not followed by whitespace. [WhitespaceAround]

I know that we seem to ignore the red crosses at pull requests, but we are working on getting them green again. Especially "checkstyle" always was taken seriously.

koppor
koppor requested changes Oct 12, 2020
View reviewed changes
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.

Looks good. Some small nitpick comments. Hope, you find time to address them.

AUTHORS Show resolved Hide resolved
src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/logic/importer/fetcher/JstorFetcher.java Outdated Show resolved Hide resolved
src/test/java/org/jabref/logic/importer/fetcher/JstorFetcherTest.java Outdated Show resolved Hide resolved
@koppor
Copy link
Member

koppor commented Oct 12, 2020

One more thing - if possible - could you add org.jabref.logic.importer.fetcher.SearchBasedFetcherCapabilityTest? That would help @DominikVoigt to add support for JSTOR for his SLR addition to JabRef. - No need do it in this PR, maybe as follow up PR? #hacktoberfest

@koppor
Copy link
Member

koppor commented Oct 14, 2020

I get 403 at a test:

Test supportsJournalSearch() FAILED

  org.jabref.logic.importer.FetcherException: A network error occurred while fetching from jstor.org/open/search?Query=pt%3A%22Test%22

  Caused by: java.io.IOException: Server returned HTTP response code: 403 for URL: jstor.org/open/search?Query=pt%3A%22Test%22

  Caused by: java.io.IOException: Server returned HTTP response code: 403 for URL: jstor.org/open/search?Query=pt%3A%22Test%22

I think, it's a permission issue. Thus, I go ahead with merging!

Thank you for the quick follow up!

@koppor koppor merged commit 170e900 into JabRef:master Oct 14, 2020
Siedlerchr added a commit that referenced this pull request Oct 14, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into winBuild
99f4de9 
* upstream/master:
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
@joethei joethei mentioned this pull request Oct 15, 2020
Merged
Siedlerchr added a commit that referenced this pull request Oct 16, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into checkstyleConfig
489cac4 
* upstream/master:
  Update journalList.mv
  Update to javafx15 (#7018)
  Squashed 'src/main/resources/csl-styles/' changes from 6fab78b..5297abd
  try to fix DEP issue with official jdk (#7008)
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
Siedlerchr added a commit that referenced this pull request Oct 17, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into writebackupToTemp
7600ea2 
* upstream/master: (58 commits)
  remove any newlines and spaces in isbn when fetching (#7023)
  add exception to error handler in integrity check
  Update journalList.mv
  Update to javafx15 (#7018)
  Squashed 'src/main/resources/csl-styles/' changes from 6fab78b..5297abd
  try to fix DEP issue with official jdk (#7008)
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
  Rewrite guidelines to Java 15 (#6973)
  Lint CHANGELOG.md
  Removing "BibTeX" when not specific to BibTeX (#6983)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koppor koppor koppor 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 resolver for jstor
3 participants
@joethei @koppor @Siedlerchr