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

Removed rdflib-jsonld as a dependency #182

Closed
wants to merge 3 commits into from
Closed

Removed rdflib-jsonld as a dependency #182

wants to merge 3 commits into from

Conversation

BryceStevenWilley
Copy link
Contributor

As of yesterday's rdflib 6.0.1 release, the rdflib-jsonld dependency is now a part of rdflib proper. This PR just removes it from the requirements.

Tests run fine on my machine (python 3.8.3, Ubuntu), let me know if there's any else I can do to help merge.

@BryceStevenWilley
Copy link
Contributor Author

I didn't quite notice that this should still be supported for python < 3.6. For a bit more clarification on this, I ran into a problem where I couldn't build a project that depended on extruct becasue rdflib-jsonld includes the use2to3 flag in setup.py, which has been very quickly deprecated and caused some issues (pypa/setuptools#2769). For python < 3.6 support, we'd need to stick with rdflib-jsonld 0.5.0, which still has the offending line. I guess the work around for packages < 3.6 is just to stick with setuptools 58.0.1. I'll close this for now, it doesn't seem particularly useful.

Copy link
Member

@lopuhin lopuhin left a comment

Choose a reason for hiding this comment

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

hi @BryceStevenWilley that makes sense, nicely spotted 👍

Please also update the packages in setup.py:

extruct/setup.py

Lines 40 to 49 in 547c8a0

install_requires=['lxml',
'rdflib',
'rdflib-jsonld',
'pyrdfa3',
'mf2py',
'w3lib',
'html-text>=0.5.1',
'six',
'jstyleson'
],

And one more thing: I approved the tests to be run and as you can see, there are some failures for older python versions.

I think it's fine to drop support for 3.5 and 3.6 now.

@lopuhin
Copy link
Member

lopuhin commented Sep 20, 2021

Sorry - I wrote the comment before you closed it but didn't post it in time. I think it would be nice to have this, but removal of p3.5 and py3.6 can be a separate PR. Let me create an issue to track this to get more feedback.

@lopuhin
Copy link
Member

lopuhin commented Sep 20, 2021

Actually thinking about it more, one more option would be to declare rdflib-jsonld a dependency which is needed only for older python versions in setup.py, and remove it from requirements.txt

@BryceStevenWilley
Copy link
Contributor Author

Hey @lopuhin, apologies for closing prematurely. I don't think this is a big enough deal to drop support for py3.5-3.6 for; rdflib-jsonld is unnecessary, but it's also harmless in 0.6.2. I like the idea of a conditional dependency in setup.py, I'll implement that later this week when I have time, and can reopen the PR when I do.

@lopuhin
Copy link
Member

lopuhin commented Sep 21, 2021

Sounds great, thank you @BryceStevenWilley

Comment on lines +5 to +6
rdflib>=6.0.1; python_version>="3.7"
rdflib<6.0.0; python_version<"3.7"
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a limbo here for version 6.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's intentional: rdflib 5.0.0 is the latest available on python 3.6 and lower, and rdflib 6.0.1 is the latest available on python 3.7, there's no good reason you would need to stick with 6.0.0, which still requires the use of rdflib-jsonld.

Copy link
Member

Choose a reason for hiding this comment

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

there's no good reason you would need to stick with 6.0.0, which still requires the use of rdflib-jsonld.

I don't think 6.0.0 requires rdflib-jsonld, see https://github.com/RDFLib/rdflib/blob/6.0.0/setup.py - so if 6.0.0 works fine for us, I'd rather not disallow it. Since extruct is a library, it makes sense to not be overly strict with what we require, to avoid conflicts with requirements of other packages.

requirements.txt Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Co-authored-by: Konstantin Lopuhin <kostia.lopuhin@gmail.com>
@lopuhin
Copy link
Member

lopuhin commented Sep 29, 2021

Looks like we have a problem on python 3.6: https://github.com/scrapinghub/extruct/runs/3744829798#step:5:25 - probably because a newer pip is already available there... Not sure what to do, except pinning an older pip/setuptools combo? That probably affects master as well. (EDIT: the issue on master is different, see below)

@lopuhin
Copy link
Member

lopuhin commented Sep 29, 2021

Checking master build here: #184 - we have a failure for 2.7, 3.5 and 3.6 but for a different reason.

@lopuhin
Copy link
Member

lopuhin commented Dec 15, 2021

hi folks, I managed to fix the 3.6 build, working on top of this PR, here #188 - what do you think about integrating the changes from there into this PR? Or we could continue in #188, at least with the review, and then merge both.

@BryceStevenWilley
Copy link
Contributor Author

I just reviewed #188, it looks good to me! Since it has the same commits as this branch, y'all should just merge it from there for a less confusing git history. :)

lopuhin added a commit that referenced this pull request Jan 17, 2022
Removed rdflib-jsonld as a dependency (from GH-182)
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

3 participants