-
Notifications
You must be signed in to change notification settings - Fork 5
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
[#295] Added support for the prefixes #298
Conversation
cefc886
to
6c85b75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment, please make dependance on spin as small as possible.
Make following method:
QueryUtils.getQueryWithModelPrefixes(String query, Model model)
Make 2 tests of this method:
- getQueryWithModelPrefixes includes all prefixes from the query
- getQueryWithModelPrefixes includes all prefixes from the model
s-pipes-core/src/test/java/cz/cvut/spipes/util/JenaUtilsTest.java
Outdated
Show resolved
Hide resolved
s-pipes-core/src/test/java/cz/cvut/spipes/util/JenaUtilsTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tests are very long and hard to read ... We are reusing staff from different PRs which does not make sense to use in this PR , e.g why are we reusing this query:
CONSTRUCT {
?airport__previous airports_from_query1:is-before-airport ?airport .
} WHERE {
#${VALUES}
}
We have CONSTRUCT query with #${VALUES}
-- this is confusing
This is also super confusing in this context ?airport__previous airports_from_query1:is-before-airport ?airport .
So why not having super easy CONSTRUCT query that makes sense, i don't know, e.g.
CONSTUCT {
?p a model-prefix:Person .
WHERE {
?p a query-prefix:Person .
}
s-pipes-core/src/test/java/cz/cvut/spipes/util/JenaUtilsTest.java
Outdated
Show resolved
Hide resolved
s-pipes-core/src/test/java/cz/cvut/spipes/util/JenaUtilsTest.java
Outdated
Show resolved
Hide resolved
s-pipes-core/src/test/java/cz/cvut/spipes/util/JenaUtilsTest.java
Outdated
Show resolved
Hide resolved
e880bfa
to
5c5a76e
Compare
@MSattrtand have a look at my last two commits: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, see my comments above.
Resolves #295