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

more type-hinting for SPARQL plugin #2265

Merged
merged 4 commits into from Mar 12, 2023

Conversation

jclerman
Copy link
Contributor

Summary of changes

Here, adding type-hints to some of the SPARQL parser plugin code.

Includes a couple of small consequent changes:

  1. Minor refactor of prettify_parsetree(), separating the public-facing callable from the internal code that does not need to be public-facing. That allows the public-facing callable to have more informative and restrictive type-hints for its arguments.
  2. Added some test-coverage for expandUnicodeEscapes() - initially for my own understanding, but seems useful to leave it in place since I didn't see test-coverage for that function.

There should be no backwards-incompatible changes in this PR - at least, not intentionally.

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
    • Considered adding an example in ./examples for new features.
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@coveralls
Copy link

coveralls commented Mar 12, 2023

Coverage Status

Coverage: 90.768% (+0.003%) from 90.766% when pulling eebb27f on jclerman:more-sparql-parser-typing into 8c48549 on RDFLib:main.

@aucampia
Copy link
Member

Thanks for the PR @jclerman, looks good from a very brief glance - I'm planning to release today, but will see if I can include this, I may make some changes directly to your branch though if needed, but i will try to keep it minimal.

@jclerman
Copy link
Contributor Author

Sure thing @aucampia. Total coincidence that I submitted this today; hadn't realized you were about to make a release - but it'd be great if this could be included! Minor changes are fine, certainly. Not sure if the flake8 violation should be fixed or allowed - I named the test after a function, and because the function had capital letters in its name, the test does too. I don't mind breaking that symmetry if we'd prefer not to add more flake8 violations.

@aucampia
Copy link
Member

Not sure if the flake8 violation should be fixed or allowed - I named the test after a function, and because the function had capital letters in its name, the test does too.

For those I will just add noqa comments, they were being ignored by flakeheaven baseline, but that resets once the code is edited, and then the generall approach is to just ignore them explicitly if they can't be fixed, or if fixing them is out of scope of the PR.

try:
return chr(int(m.group(1), 16))
except: # noqa: E722
raise Exception("Invalid unicode code point: " + m)
except (ValueError, OverflowError) as e:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if OverflowError could be raised from here, not that it is that much of a problem if it can't but would like to know just for interest’s sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my local testing I found that OverflowError could be raised for large 8-digit hexadecimal codepoint numbers - apparently because they can be larger than a 32-bit int. I'm not sure why the max isn't 64-bit, but that might be a limitation caused by the use of int().

Valid codepoints can't be larger than \U10000000, so it's kind of a moot point, except that due to the way the regex is written, the function will try to handle \U99999999 and will trigger OverflowError.

Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

I made a very small change, adding BinaryIO in addition to TextIO, as that should also work. Everything looks great otherwise, thanks again for this.

@aucampia aucampia requested a review from a team March 12, 2023 12:06
@aucampia aucampia added review wanted This indicates that the PR is ready for review ready to merge The PR will be merged soon if no further feedback is provided. labels Mar 12, 2023
@aucampia
Copy link
Member

I plan to merge with only one review unless there is more feedback soon, as I want to finish the release today and there are more changes I want to get in for SPARQL typing, as the current typing in Graph.{query,update} is too strict.

@aucampia aucampia merged commit a44bd99 into RDFLib:main Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants