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

issue_771_add_key_error_if_spaces #1070

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

abhishekvickyvyas
Copy link

@abhishekvickyvyas abhishekvickyvyas commented May 26, 2020

Hello,
Here is my contribution for issue #771 . I raise a key error if prefix have spaces in it. The changes are done is /rdflib/NamespaceManger.bind. Please let me know if error message need to be change or if we need to raise only warning instead of error message.

@coveralls
Copy link

coveralls commented May 26, 2020

Coverage Status

Coverage increased (+0.05%) to 75.473% when pulling 4276aa7 on abhishekvickyvyas:master into 7a53c61 on RDFLib:master.

@abhishekvickyvyas abhishekvickyvyas changed the title issue_771_add_key_error_if_spaces Solution for issue #771 issue_771_add_key_error_if_spaces May 27, 2020
@abhishekvickyvyas abhishekvickyvyas changed the title Solution for issue #771 issue_771_add_key_error_if_spaces issue_771_add_key_error_if_spaces May 27, 2020
@nicholascar
Copy link
Member

nicholascar commented Aug 13, 2020

Hi @abhishekvickyvyas thanks for addressing this - it's a real error in RDFlib - but your solutions is odd! Perhaps you're using a JavaScript code style in Python??

Rather than:

 if (len(prefix.split(" ")) > 1):
    raise KeyError("\'" + prefix + "\' does not look like a valid prefix as it has spaces, change it into correct format")

How about:

 if " " in prefix:
    raise KeyError("'{}' does not look like a valid prefix as it has spaces".format(prefix))

We do also need a test for any PR (RDFlib policy) so you'll need to write a small test function for this.

If you make this change, and add a test, we'll accept the PR. Thanks (maintainers)

@nicholascar nicholascar merged commit b41560f into RDFLib:master Mar 5, 2021
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.

4 participants