Feature/sparql format #621
Conversation
There was a problem hiding this comment.
how precise should the validation of parameters be? In sparql::is_value_valid() the SparqlParameter::Endpoint and SparqlParameter::Query are validated intensely, which seems like a lot of effort.
Even if this takes a bit of effort, we're doing this once, when loading the program, so the performance overhead is negligible, especially if we have queries with lots of results. Sending off a query to the endpoint, only to then find out that it fails because there's a syntax error, however, is certainly going to take much longer.
The parsing of a resource (now can be either IRI or string) was moved to format::ResouceSpec::parse_string while before, the two file providers HttpResourceProvider and FileResourceProvider were handling this.
As mentioned, this should go into a TryFrom<Resource> for ResourceSpec.
in format_builder::new_with_tag the created resource: Option can be replaced by a Option returned from any Builder. This allows the SparqlBuilder to return its own ResourceSpec that uses endpoint and query.
As mentioned, this should rather be another method on the trait, with a default implementation that does not replace the ResourceSpec.
in resource_providers::open_resource() the existing vector containing both ResourceProviders is used. -> can this be simplified somehow?
This should stay as it is. The fact that there's only two entries is an implementation detail that we can't rely on (and might not be true in, e.g., some test cases).
Laschoking
force-pushed
the
feature/sparql-format
branch
from
8eb375d to
f6560ed
Compare
…urning a Resource The commit misses the actual reading of parameters (get_parameters, header_parameters, body_parameters, fragment) from an import statement. However, the ResourceBuilder provides all needed functionalities to include them.
…ResourceProvider and HttpResourceProvider
There was a problem hiding this comment.
This is going in the right direction. I have a lot of comments, but most of them are stylistic issues (particularly in user-facing code such as error messages).
You can ignore the clippy issues for now and rebase once #626 is merged, they're already fixed there. The doc failure is already fixed, next run should work again.
| @@ -407,9 +441,77 @@ impl ImportExportBuilder { | |||
| } | |||
| }?; | |||
|
|
|||
| let resource_builder = inner | |||
| .override_resource_builder(direction) | |||
| .or(resource_builder); | |||
There was a problem hiding this comment.
Why not make this unwrap_or(resource_builder); and get rid of an if let with an else that is way too far along, below?
| .to_iri() | ||
| .ok_or(ValidationErrorKind::ImportExportInvalidIri) | ||
| .and_then(|iri| { | ||
| Iri::parse(iri) | ||
| .map(|_| ()) | ||
| .map_err(|_| ValidationErrorKind::ImportExportInvalidIri) | ||
| }), |
There was a problem hiding this comment.
This shouldn't need to throw the same error twice, clearly, to_iri() should actually return a validated IRI.
With this changes a new keyword
sparqlis introduced that expects two keywords:endpointis expected to be a valid IRI andquerya String.f.e.
@import test :- sparql{endpoint=<https://query.wikidata.org/>, "SELECT ?item ..."}.Furthermore, resources can be written as IRIs now.
Most importantly the Resource-type is changed, allowing now a
Resource::Irifor HTTPRequests (either online-files or sparql queries) andResource::Pathfor local resources.Known lacks:
Questions for review:
sparql::is_value_valid()theSparqlParameter::EndpointandSparqlParameter::Queryare validated intensely, which seems like a lot of effort.format::ResouceSpec::parse_stringwhile before, the two file providersHttpResourceProviderandFileResourceProviderwere handling this.-> is this change agreeable?
format_builder::new_with_tagthe createdresource: Option<ResourceSpec>can be replaced by aOption<ResourceSpec>returned from any Builder. This allows theSparqlBuilderto return its own ResourceSpec that usesendpointandquery.-> is this agreeable or should it be handled elsewhere?
resource_providers::open_resource()the existing vector containing bothResourceProvidersis used. -> can this be simplified somehow?