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

Fix mypy type errors and add mypy to .drone.yml #1407

Merged
merged 3 commits into from Sep 27, 2021

Conversation

aucampia
Copy link
Member

Fixes #1311

Add mypy to .drone.yml and fix type errors that come up.

Not type checking examples or tests.

Other changes:

Fixes RDFLib#1311

Add `mypy` to .drone.yml and fix type errors that come up.

Not type checking examples or tests.

Other changes:

- fix return value for `Graph.serialize` (refs RDFLib#1394)
- remove default value for
  `rdflib.plugins.sparql.algebra.translateAlgebra` (refs RDFLib#1322)
- add .dockerignore to reduce context size and make docker quicker
  to run.
- add .flake8 config to ignore line length as black is managing
  formatting.
- add mypy to docker-compose, makefile and tox.ini
- fix the way csv2rdf is invoked to ensure that the right code gets
  executed.
@aucampia
Copy link
Member Author

@nicholascar This change will make drone fail if mypy fails, if you want I can change it so that errors are discarded (i.e. add || true).

Copy link
Member

@nicholascar nicholascar left a comment

Choose a reason for hiding this comment

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

Looks sensible to me so over to @ashleysommer / @white-gecko to just check the mypy / drone implications.

@nicholascar nicholascar requested review from white-gecko and removed request for edmondchuc September 11, 2021 00:11
`Graph.serialize` was just logging a warning if the supplied `destination`
was not valid, but it would be better to fail if the method does not
perform the requested operation.

This change makes `Graph.serialize` throw if the supplied `destination`
can't be written to.
@aucampia
Copy link
Member Author

aucampia commented Sep 12, 2021

I included the following as per discussion in #1394

Change Graph.serialize to throw for invalid destinations

Graph.serialize was just logging a warning if the supplied destination
was not valid, but it would be better to fail if the method does not
perform the requested operation.

This change makes Graph.serialize throw if the supplied destination
can't be written to.

Don't mind moving this to a separate PR if desired.

@nicholascar
Copy link
Member

All good with the inclusions of the error raise in this PR. @ashleysommer can you please take a look?

@ajnelson-nist
Copy link
Contributor

I hope you'll pardon my butting in. I've been trying mypy in some other projects. I've found some cases require the addition of a py.typed file, with some extra setup references needed to export the py.typed file. One place I did this is here. I did this from reviewing PEP 561 and from finding it necessary from some tinkering.

Is it more appropriate for rdflib to have, or not to have, a py.typed file?

@aucampia
Copy link
Member Author

I hope you'll pardon my butting in. I've been trying mypy in some other projects. I've found some cases require the addition of a py.typed file, with some extra setup references needed to export the py.typed file. One place I did this is here. I did this from reviewing PEP 561 and from finding it necessary from some tinkering.

Is it more appropriate for rdflib to have, or not to have, a py.typed file?

I was not aware of this, thanks for the info. I think it should have py.typed in this case then. I will try test this and see what is up, maybe make another PR to add it.

@nicholascar
Copy link
Member

From the PEP: “if a top-level package includes it, all its sub-packages MUST support type checking as well.”

So please do go ahead and add a py.typed file but just consider where it should be added. I assume to the rdflib/ folder, not the repo parent folder!

Unless test/ also needs one…

Need to get rid of nose too… Ugh, lots to do…

This is so that other packages importing rdflib does not get warnings
like this:

```
$ python3.9 -m mypy --show-error-context --show-error-codes  test_rdflib.py
test_rdflib.py:1: error: Skipping analyzing "rdflib": found module but no type hints or library stubs  [import]
test_rdflib.py:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)
```
@aucampia
Copy link
Member Author

aucampia commented Sep 21, 2021

So please do go ahead and add a py.typed file but just consider where it should be added. I assume to the rdflib/ folder, not the repo parent folder!

I added it in rdflib/ - and put it on this PR. I think it only really matters for what is distributed in wheels anyway, so even if we enforce typing in test/ it is not needed there as others should not be importing tests.

This will prevent the following error from occuring:

$ python3.9 -m mypy --show-error-context --show-error-codes  test_rdflib.py
test_rdflib.py:1: error: Skipping analyzing "rdflib": found module but no type hints or library stubs  [import]
test_rdflib.py:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)

test_rdflib.py:

import rdflib

# python3.9 -m mypy --show-error-context --show-error-codes --strict --allow-untyped-calls test_rdflib.py

g = rdflib.Graph()

Thanks again for the heads up on this @ajnelson-nist

@nicholascar nicholascar merged commit b07977c into RDFLib:master Sep 27, 2021
ajnelson-nist added a commit to casework/CASE-Utilities-Python that referenced this pull request Oct 7, 2021
This patch is to show what is needed to add static type checking with
`mypy`.

The brunt of the effort is adding `#type: ignore` annotations to
`rdflib`.  These can be removed once an `rdflib` release with the merged
PR 1407 is issued.

The additional package in `tests/requirements.txt` pertaining to
`dateutil` was reported by `mypy`.

DISCLAIMER:
Participation by NIST in the creation of the documentation of mentioned
software is not intended to imply a recommendation or endorsement by the
National Institute of Standards and Technology, nor is it intended to
imply that any specific software is necessarily the best available for
the purpose.

References:
* [AC-211] Add static type checking to CASE-Utilities-Python
* RDFLib/rdflib#1407

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
ajnelson-nist added a commit to casework/CASE-Utilities-Python that referenced this pull request Oct 7, 2021
One more `# type: ignore` was added while awaiting rdflib PR #1407.

These were all identified by adding the `--strict` flag to mypy for a
run.  I will leave it up for future discussion whether to use that flag,
especially to wait for PR 1407 and to see if too much work would be
induced versus runtime safety improvements.

References:
* [AC-211] Add static type checking to CASE-Utilities-Python
* RDFLib/rdflib#1407

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist
Copy link
Contributor

I'm glad my feedback was helpful!

I look forward to seeing a release with this PR. Adding type checking to my RDF projects still requires # type: ignore on all of my rdflib imports, though I feel confident that won't be needed much longer.

Comment on lines 365 to -368
def namespaces(self):
""" """
if False:
yield None
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry I didn't get a chance to review this before it was merged.
Why was this changed, it has implicates for all Store implementations that don't implement namespaces().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I just saw #1432, with an explanation.

@aucampia aucampia deleted the iwana-20210910T2303 branch November 17, 2021 19:21
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.

Policy on type annotations
5 participants