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

DOI determining now uses "DOI.findInText" (and not "DOI.parse") #8227

Closed
wants to merge 1 commit into from

Conversation

koppor
Copy link
Member

@koppor koppor commented Nov 8, 2021

This addresses #8127 by using the findInText functionality of the DOI parsing functionaltiy. Thereby, my copy&paste issue is gone.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor
Copy link
Member Author

koppor commented Nov 8, 2021

Waiting for @jiezheng5 to include their test cases from #8215 to here. findInText should be used instead of parse

@mrcstan
Copy link
Contributor

mrcstan commented Nov 9, 2021

@koppor, @Siedlerchr, I found a case where this method failed in Line 120 in DOITest.java. For the DOI, https : / / doi.org / 10 / gf4gqc , what I got with @koppor's proposed change is the following:

  1. After copy and pasting the DOI,
    image

  2. After pressing enter,
    image

In contrast, the proposed change in the PR #8228 did not have the issue

image

@jiezheng5
Copy link
Contributor

jiezheng5 commented Nov 9, 2021

Waiting for @jiezheng5 to include their test cases from #8215 to here. findInText should be used instead of parse

I would like to add the Parameterized Test Cases here. However, since that this PR(#8127) is not merged nor sent by me, I don't know how I can modify it locally.
I tried
image

thanks very much for your time.

@Siedlerchr
Copy link
Member

Siedlerchr commented Nov 9, 2021

@jiezheng5 If jabRef/jabref is configured as upstream then: (git remote -v)

git fetch upstream --prune
git checkout fix-8127

@@ -21,7 +21,7 @@ public CompositeIdFetcher(ImportFormatPreferences importFormatPreferences) {
}

public Optional<BibEntry> performSearchById(String identifier) throws FetcherException {
Optional<DOI> doi = DOI.parse(identifier);
Optional<DOI> doi = DOI.findInText(identifier);
Copy link
Member

Choose a reason for hiding this comment

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

Semantically, you would want to parse the identifier here and not find it in some bigger text. Thus, if you think "findInText" is better, then please change the implementation of "parse". This makes it also more coherent with the other id fetchers below that use "parse" as well (and the idea was to extract this to a common interface at some point).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on the parser in the PR #8228

Copy link
Contributor

Choose a reason for hiding this comment

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

I use single regexp to parse the DOI in and added several test cases in DOITest PR #8228. Also address the request by @koppor for informing the user that "no DOI data exists" rather than "invalid DOI" when the fetcher fails to download from a valid DOI (#8217).

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the approach by @mrcstan in #8228.

Copy link
Member

@ThiloteE ThiloteE Nov 19, 2021

Choose a reason for hiding this comment

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

@mrcstan

I use single regexp to parse the DOI in and added several test cases in DOITest PR #8228. Also address the request by @koppor for informing the user that "no DOI data exists" rather than "invalid DOI" when the fetcher fails to download from a valid DOI (#8217).

i think you meant to reference #8127, not 8217. I just got confused when i followed the link in 8217 to this thread. This one here deals with an entirely different topic.

@jiezheng5
Copy link
Contributor

@jiezheng5 If jabRef/jabref is configured as upstream then: (git remote -v)

git fetch upstream --prune
git checkout fix-8127

Thanks very much for the response. I encountered the following error following the instructions.
image

sincerely,

@koppor
Copy link
Member Author

koppor commented Nov 10, 2021

Fully agree with @tobiasdiez - following up at #8228.

@koppor koppor closed this Nov 10, 2021
@koppor koppor deleted the fix-8127 branch November 10, 2021 21:20
@koppor
Copy link
Member Author

koppor commented Nov 10, 2021

@jiezheng5 Regarding your git issue, it seems that your configuration of upstream is wrong:

grafik

You should execute

git remote set-url upstream https://github.com/jabref/jabref.git

However, check before

git remote -v

to check which repositories you have configured

@jiezheng5
Copy link
Contributor

@jiezheng5 Regarding your git issue, it seems that your configuration of upstream is wrong:

grafik

You should execute

git remote set-url upstream https://github.com/jabref/jabref.git

However, check before

git remote -v

to check which repositories you have configured

thanks very much, it worked now.

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.

6 participants