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

update Papis Sci-Hub plugin #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

raj-magesh
Copy link

I've created a Sci-Hub Downloader that's loaded as an Importer entry point. I've been using it without issues. Please let me know if this is fine.

One thing to note is that this downloads BibTeX metadata directly from https://doi.org and some of the fields that I get from the DOI importer are missing. The formatting is also slightly different (e.g. the year field is '2019' instead of 2019. It's fine for my purposes but let me know if there's a better, more consistent way to do this.

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert and fixes 2 when merging c50f34d into 4d07504 - view on LGTM.com

new alerts:

  • 1 for Incomplete URL substring sanitization

fixed alerts:

  • 1 for Unused local variable
  • 1 for Insecure temporary file

@alejandrogallo
Copy link
Member

Hello @raj-magesh thank you very much for this, I'll test it but I'm not using python 3.10.
Is there a way that you can make it compatible at least with python 3.6 upwards easily?
I.e. for instance not using f strings?

Papis tries to be compatible with py36 upwards, maybe it'll change in the future.

Cheers!

@raj-magesh
Copy link
Author

Sorry, I've been busy, but I'll get to this when I get some time! Shouldn't be too hard to port to Python 3.6.

@hseg
Copy link

hseg commented Nov 12, 2023

While trying to package this for Arch Linux, noticed the setup.py->pyproject.toml conversion dropped the version info (eg dynamic = ["version"] only works if the repo actually tags versions), and forgot to add a [tool.setuptools_scm] section.
BTW, this backwards compatibility policy is exceedingly generous -- 3.7 is already EOL, and 3.6 was EOL when the above comments were written. Moreover, I don't see the issue as initially mentioned -- f-strings are a python 3.6 feature, as requested.

Attached is a patch on top of the above making it pylint --py-version 3.6-clean (mod docstrings) and addressing the above concerns.
(renamed to txt to pass github's filters)
fix-build-lint.txt

@alejandrogallo
Copy link
Member

In the meantime we decided to drop 3.6, yes, so this should be accepted, I just have no time resources now to test this specific pull request, is someone available to referee this pr and approve the merge? Maybe @hseg you can add your diff to the pr?

@hseg
Copy link

hseg commented Nov 15, 2023 via email

@hseg
Copy link

hseg commented Nov 15, 2023

Correction: got confused by the extensive rewrite of plugin.py which made it seem like a rebase was necessary. No edits have been made to papis-scihub since the PR was opened, so it rebased cleanly.

@hseg
Copy link

hseg commented Nov 15, 2023

However, just noticed PR also removes the liability warning, restoring it.

@hseg
Copy link

hseg commented Dec 6, 2023

@alejandrogallo ping on merging? (or, well, merging #62)
It's been nearly a month.

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.

3 participants