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

add support for in expression #795

Merged
merged 8 commits into from
Jun 17, 2024

Conversation

dpetran
Copy link
Contributor

@dpetran dpetran commented Jun 4, 2024

Semantics and syntax from SPARQL IN expression.

@dpetran dpetran requested a review from a team June 4, 2024 20:10
@@ -285,9 +285,13 @@
[x]
(str x))

(defn sparql-in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we need a better name for this function because it's used in fql as well as sparql

@@ -440,10 +440,18 @@
test-utils/default-str-context
{"ex" "http://example.com/"}]
"where" [{"id" "?s", "ex:text" "?text"}
["bind" "?bound" "(bound ?text)"]]
"insert" {"id" "?s", "ex:bound" "?bound"}
["bind"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talk a lot about improving the test commentary in code reviews. This test is one of the worst offenders. The whole "functional forms" section is bad so it's probably outside the scope of this specific pr to make it better, but it takes entirely too much effort to figure out what's happening in this test if you made a code change that breaks it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've been trying to update the bad ones as I go but this one is a lot of work to refactor, so I skipped it. But, it's not hard work, so I'll see if I can refactor the whole thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said, the existing tests are outside the scope of this particular pr so I don't think you have to refactor the whole thing, but new test assertions added here should still provide enough information for someone else later to be able to make sense of what's being tested, what the expected behavior is, and, in the case of something breaking, what broke and in what way it broke.

@dpetran dpetran force-pushed the feature/fql-exists-not-exists branch from cee17e3 to 521d4c3 Compare June 12, 2024 21:57
Semantics and syntax from SPARQL IN expression.
It's not sparql-specific, so no need to qualify it.
The :Expression parser was turning everything into a string, which works in some cases
but introduces unwanted quoting in others. By deferring stringification until values
need to be incorporated into an actual string expression we avoid the unwanted quoting.

Do note that all the numeric functions now correctly expect numeric values instead of
strings.
abs expects only a single expression as an argument, and that argument does not need to
be quoted.
translate SPARQL IN/NOT IN expresssions into FQL
@dpetran dpetran merged commit 4c13b1b into feature/fql-exists-not-exists Jun 17, 2024
7 checks passed
@dpetran dpetran deleted the feature/fql-in-not-in branch June 17, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants