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

enhance warning for missing rdflib-jsonld #38

Closed
joernhees opened this issue Jul 31, 2014 · 8 comments
Closed

enhance warning for missing rdflib-jsonld #38

joernhees opened this issue Jul 31, 2014 · 8 comments
Assignees
Milestone

Comments

@joernhees
Copy link
Member

Currently a default install of SPARQLWrapper issues this warning for every user:

In [1]: from SPARQLWrapper import SPARQLWrapper, JSON
/usr/local/lib/python2.7/site-packages/SPARQLWrapper/Wrapper.py:100: RuntimeWarning: JSON-LD disabled because no suitable support has been found
  warnings.warn("JSON-LD disabled because no suitable support has been found", RuntimeWarning)

Several issues with this:

  • seems weird to me... if i just installed something, then want to use it and the first thing it tells me is "Warning, i'm not complete" i get a bad feeling...
  • if missing jsonld support is worth a warning, why doesn't SPARQLWrapper depend on it so it's auto installed?
  • in any case the warning should tell me that i maybe want to install the rdflib-jsonld package... this might seem obvious to us, but newcomers might be quite confused without this hint.
  • if it's really optional (so we don't expect that a sane endpoint unasked replies with JSONLD), could this warning maybe be delayed until someone tries tosetReturnFormat(JSONLD)?
@indeyets
Copy link
Contributor

I'm not a big fan of warnings. I don't see much value in them. I think it's better to just work until we can't at which point exception should be raised. Warnings are used to mask inconsistencies in design.

I think it is ok to tweak this for 1.7 (no need to change behaviour in 1.6). I would prefer to get rid of warnings in 2.x

@mgaldzic
Copy link

I agree - this warning bugs me too.
Should be thrown as an exception when I try to retrieve JSON-LD but can not.

On Thu, Jul 31, 2014 at 1:23 PM, Alexey Zakhlestin <notifications@github.com

wrote:

I'm not a big fan of warnings. I don't see much value in them. I think
it's better to just work until we can't at which point exception should be
raised. Warnings are used to mask inconsistencies in design.

I think it is ok to tweak this for 1.7 (no need to change behaviour in
1.6). I would prefer to get rid of warnings in 2.x


Reply to this email directly or view it on GitHub
#38 (comment).

@wikier
Copy link
Member

wikier commented Aug 4, 2014

The reason why that's a warning it's because there is not reason in the protocol whether a SPARQL endpoint should provide JSON-LD support.

The warning is not shown at request time, just at the wrapper initialization to dynamically setup the allowed formats.

Therefore it can't be an exception. But I'm happy to list to idea how to properly warn the user about such limitation of the environment where the code runs. From my point of view these are the options:

  • keep the warning, so the user notice it and can install such optional dependency
  • move the warning to setReturnFormat() (now silently ignores invalid formats)
  • remove the warning at all

I'm not happy with the last one, but the other two options sound good for me.

@wikier wikier added this to the 1.6.3 milestone Aug 4, 2014
@wikier wikier self-assigned this Aug 4, 2014
@indeyets
Copy link
Contributor

indeyets commented Aug 4, 2014

@wikier I think the consensus is, that it's better to remove the warning and throw exception just when this functionality is requested (but can't be provided).

additionally, there might be a method for explicit check:

def supports(functionality: str) -> bool:
    if str == JSONLD:
        return JSONLD in _allowedFormats

(I used py3k type-hints syntax to clarify intentions, obviously, we can't have it in actual code)

@wikier
Copy link
Member

wikier commented Aug 5, 2014

OK, according the feedback, the changes has been introduced. Please @joernhees, @indeyets and @mgaldzic, check if they implement the expected behavior to close this issue.

@joernhees
Copy link
Member Author

@wikier 👍 much better, i left an idea for improvement in the code which raises the exception

@wikier
Copy link
Member

wikier commented Aug 5, 2014

I think we can consider this resolved, isn't it?

@wikier wikier closed this as completed Aug 5, 2014
@joernhees
Copy link
Member Author

👍 thanks a lot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants