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

ambigous name: "addCustomParameter" #1

Closed
indeyets opened this issue Nov 8, 2013 · 12 comments
Closed

ambigous name: "addCustomParameter" #1

indeyets opened this issue Nov 8, 2013 · 12 comments
Assignees
Labels
Milestone

Comments

@indeyets
Copy link
Contributor

indeyets commented Nov 8, 2013

name of addCustomParameter method suggests that parameter would be added to the list, while implementation actually replaces existing parameter with the same name.

SPARQL specification allows to have multiple parameters with the same name given to the endpoint (for example named graphs).

…and may include zero or more default graph URIs (parameter name: default-graph-uri) and named graph URIs (parameter name: named-graph-uri)

Changing behaviour of addCustomParameter would break backwards compatibility, so the proper solution would be to:

  • introduce setCustomParameter method which does what current implementation does
  • introduce appendCustomParameter which adds more parameters with the same name
  • deprecate addCustomParameter
@joernhees
Copy link
Member

I agree that this is confusing, not sure how much it would break though if we just fixed it.

@indeyets
Copy link
Contributor Author

indeyets commented Nov 8, 2013

well… I supposed there might be users who rely on ability to change parameters in runtime and as there is no other method for that right now their apps will fail if behaviour is different

other possibility is to introduce boolean replace_existing parameter to method defaulting to True

@ghost
Copy link

ghost commented Nov 8, 2013

Off-topic probably but I can't make sense of "zero or more default graph URIs". Can there be multiple defaults?

(a bare "yes" is an okay answer, I can quench my incredulity with the details later.)

@indeyets
Copy link
Contributor Author

indeyets commented Nov 8, 2013

@gjhiggins yes :) see this exampe

@indeyets
Copy link
Contributor Author

indeyets commented Nov 8, 2013

while this one is still opened… would using addParameter and setParameter instead of addCustomParameter help? current API suggests to use addCustomParameter for everything, so it is not "custom" anymore

@ghost
Copy link

ghost commented Nov 8, 2013

@indeyets - aha, many thanks.

"The RDF dataset description is composed of zero or one default RDF graphs — composed by the RDF merge of the RDF graphs identified by zero or more default-graph-uri (types)"

I knew there had to be more to it.

@ghost ghost assigned wikier Nov 11, 2013
@wikier
Copy link
Member

wikier commented Nov 11, 2013

The actual reason behind the current implementation is that we were not aware of the possibility of multiple parameters with the same name, not in SPARQL Protocol, but in HTTP.

Therefore I agree it could be confusing. But what I don't have clear is how to solve it... the idea with addCustomParameter was to avoid validation of some custom parameters, but that's something could be internally handled.

So I'd prefer to switch to single method addParameter where some kind of validation would be done for normative parameters, none for custom ones. Personally I'd prefer not to add setParameter while this is something not strictly necessary from an implementation point of view.

What do you think?

@indeyets
Copy link
Contributor Author

setParameter is not needed as long as there is a way to clear parameter (remove all added values). The object of class is reusable so there is a need specify different parameters for the next query (preferrably without resetting everything)

@wikier
Copy link
Member

wikier commented Nov 12, 2013

I'd prefer to have a clearParameter than a setParameter for such purpose.

@wikier
Copy link
Member

wikier commented Dec 14, 2013

If everybody is happy with the current solution, I'll close the issue.

@indeyets
Copy link
Contributor Author

👍

@wikier
Copy link
Member

wikier commented Jan 12, 2014

so we can consider this issue closed

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

No branches or pull requests

3 participants