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

Do not drop .pdf urls #704

Closed
wants to merge 31 commits into from
Closed

Do not drop .pdf urls #704

wants to merge 31 commits into from

Conversation

GlazerMann
Copy link
Collaborator

Includes &uid= on end

Fixes deleting open acces copies

Includes &uid= on end
@ms609
Copy link
Owner

ms609 commented Sep 1, 2018

We should include a test case here with an unambiguously necessary URL, so that this does not get removed in future. (I'm still keen to see an example that cannot simply be reached through the DOI link.)

@codecov
Copy link

codecov bot commented Sep 1, 2018

Codecov Report

Merging #704 into master will decrease coverage by 1.42%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #704      +/-   ##
============================================
- Coverage     69.79%   68.37%   -1.43%     
- Complexity     1624     1672      +48     
============================================
  Files            12       12              
  Lines          3364     3377      +13     
============================================
- Hits           2348     2309      -39     
- Misses         1016     1068      +52
Impacted Files Coverage Δ Complexity Δ
Template.php 73.63% <100%> (-2.37%) 1490 <81> (+48)
DOItools.php 93.75% <0%> (+0.09%) 0% <0%> (ø) ⬇️
expandFns.php 90.97% <0%> (+0.69%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77785b8...90aca29. Read the comment docs.

Copy link
Owner

@ms609 ms609 left a comment

Choose a reason for hiding this comment

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

Do we need to use preg_match here? Would we be better using a stripos and a constant in a suitable file?

Template.php Outdated Show resolved Hide resolved
ms609
ms609 previously requested changes Sep 5, 2018
tests/phpunit/TemplateTest.php Outdated Show resolved Hide resolved
tests/phpunit/TemplateTest.php Outdated Show resolved Hide resolved
@GlazerMann
Copy link
Collaborator Author

I probably should use real urls

@GlazerMann
Copy link
Collaborator Author

GlazerMann commented Sep 5, 2018

I made up the urls since I have yet to find a journal PDF url that ends in .pdf. Thus, the second test does not occur. The real url must: have the doi, end in .pdf, and be from a publisher.

@GlazerMann
Copy link
Collaborator Author

Todo: better URLs

@ms609
Copy link
Owner

ms609 commented Sep 6, 2018

Could it be that a .pdf ending is enough to signify that a link is not from a publisher, after all?

@GlazerMann
Copy link
Collaborator Author

I have improved the user output. I agree that the publisher list is overkill, but we do know that publishers do have pdf files

@GlazerMann
Copy link
Collaborator Author

@ms609 I think this is ready to go.

@ms609
Copy link
Owner

ms609 commented Sep 11, 2018

I still think that without real URLs to use as tests, there's no proven value behind this additional code complexity.

@GlazerMann
Copy link
Collaborator Author

complexity removed @ms609

@ms609
Copy link
Owner

ms609 commented Sep 12, 2018

Could it be that a .pdf ending is enough to signify that a link is not from a publisher, after all?

Against this assertion, here's a real life publisher link ending in .pdf (but not containing a doi...) http://jcs.biologists.org/content/joces/s3-100/49/89.full.pdf

@GlazerMann
Copy link
Collaborator Author

I guess we just accept this pull as it is, and wait for an example of a URL that:

  1. contains DOI
  2. ends in PDF
  3. is a publisher's website
    Once we find one of those, start forgetting those urls

@GlazerMann GlazerMann dismissed ms609’s stale review September 13, 2018 21:08

Unneeded code removed

Copy link
Owner

@ms609 ms609 left a comment

Choose a reason for hiding this comment

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

Do we need to use process_citation in the tests, or will prepare_citation do the trick?

Does the free-floating URL duplicate an existing test? Can we in fact do without the first of the three tests, whcih duplicates the second?

@GlazerMann
Copy link
Collaborator Author

prepare_citation() did the trick

@GlazerMann GlazerMann closed this Sep 19, 2018
@GlazerMann GlazerMann deleted the patch-14 branch September 19, 2018 21:35
@GlazerMann GlazerMann mentioned this pull request Sep 25, 2018
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.

None yet

2 participants