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

Remove dependency on rdflib-jsonld #94

Merged
merged 2 commits into from
Sep 13, 2021
Merged

Conversation

Panaetius
Copy link
Contributor

Closes #93

Requires RDFLib/OWL-RL#46 to be merged and released first, then needs a poetry lock.

Isort reordered some imports in pyshacl/validate.py, I can undo that but as it is right now it should follow the CONTRIBUTING guidelines of the project.

@ashleysommer
Copy link
Collaborator

I don't know why Black changed so many lines in various unrelated files in the commit.
I ran re-black and isort on the whole codebase two months ago when I updated the required version of Black to 21.6b0, so they should all already be in the correct state.

Did you run with the given Black config file? black --config=./pyproject.toml ./pyshacl?

Regarding isort, that has a mind of its own. It seems to reorder imports on random files all the time, and a couple of commits later it will re-order them back..

@Panaetius
Copy link
Contributor Author

Panaetius commented Sep 10, 2021

I did

$ pip install black==21.6b0
$ black --verbose --config ./pyproject.toml pyshacl

in the virtual env for the project (same with poetry run black ...).

I believe this is correct behavior, see https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#trailing-commas . Black will split a collection of arguments to separate lines if they and in a trailing comma. I remember there used to be a bug in black where, if you call it, it'd add a trailing comma to these kinds of argument lists, which would then cause it to break them onto separate lines if run again (so running black twice didn't yield the same result as running it once). This has since been fixed, but maybe you ran into that issue?

On a side note, we've had good experiences using pre-commit with this config file: https://github.com/SwissDataScienceCenter/renku-python/blob/master/.pre-commit-config.yaml and mentioning in our CONTRIBUTING.md that a user should do pre-commit install. Then black will complain if a user tries to commit a file that wasn't properly formatted. Just a small quality of life improvement you might be interested in.

Edit:
psf/black#1629 this was the black issue, that was in 20.8b0

As for isort, I think it doesn't sort imports past non-import code, so me removing

if owlrl.json_ld_available:
    import rdflib_jsonld  # noqa: F401

caused it to also pickup the imports below that line and sort them

@ashleysommer
Copy link
Collaborator

ashleysommer commented Sep 11, 2021

psf/black#1629 this was the black issue, that was in 20.8b0

Yep, looks like you're right. this is that bug. I had pinned our version of black to 20.6b0 back in June because that was the first version which contained the required-version config param, so we could enforce a specific version of black being used.

I'll update it to v20.8b0 to fix that bug.

@ashleysommer
Copy link
Collaborator

@Panaetius
PySHACL v0.16.2 is released, with the ImportError bug fixed, and the required version of Black bumped to 21.8b0 to fix the black bug. Reblacked all source files, which brought in the same formatting changes as seen in this PR.

This PR should now be able to be rebased against current Master to make the changeset smaller.

@ashleysommer ashleysommer self-requested a review September 12, 2021 22:25
Copy link
Collaborator

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

Please rebase or merge against current master

@Panaetius
Copy link
Contributor Author

I've rebased on current master.

@ashleysommer ashleysommer marked this pull request as ready for review September 13, 2021 07:40
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.

Remove (soft)dependency on rdflib_jsonld
2 participants