-
Notifications
You must be signed in to change notification settings - Fork 25
Adds test to prevent repeated variables in VALUES #225
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
base: main
Are you sure you want to change the base?
Conversation
d8f23a5
to
8cac1ca
Compare
<dd>mf:PositiveEntailmentTest</dd> | ||
<dt>approval</dt> | ||
<dd property='mf:approval' resource=''></dd> | ||
<dd property='mf:approval' resource=''>none</dd> |
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.
This may become unnecessary if another PR is merged. This looks like it is the report generator catching up.
@@ -0,0 +1 @@ | |||
SELECT * WHERE { VALUES (?a ?a) { (1 1) } } |
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.
I don't think this is the right place.
- It isn't an expression.
- All syntax tests go in a directory name of the form
syntax-*
, separate from eval tests.
Suggestion: start a new directory: sparql/sparql12/syntax
for both positive syntax-*.rq
tests and syntax-bad-*.rq
for syntax tests that do not warrant their own directory.
Discussion: should this be in sparql11
?
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.
Indeed, a syntax folder would be great. Will do so.
Discussion: should this be in sparql11?
imho it's undefined behavior in SPARQL 1.1 and is only defined in SPARQL 1.2 so an implementation doing something else would be still compliant with SPARQL 1.1. This is why I put this test in SPARQL 1.2.
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.
This is why I put this test in SPARQL 1.2.
OK
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.
Moved into a new syntax dir
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.
Looks good, thanks @Tpt!
I do agree with moving this to a syntax-
directory.
I also see this as something that should be in sparql12
.
Test for w3c/sparql-query#294