Adjust Query Representation and SPARQL Query for without state#70
Conversation
guergana
changed the title
|
@Ladsgroup here, ready for review. |
| }, | ||
| ]; | ||
|
|
||
| if ( queryRepresentation.condition.propertyValueRelation === PropertyValueRelation.NotMatching ) { |
There was a problem hiding this comment.
This method is getting quite long. But I think it is still ok and we can refactor it when we get to the multiple conditions.
| const propertyValueRelation = PropertyValueRelation.NotMatching; | ||
|
|
||
| /* | ||
| * adding an array here to circumvent backticks + indentation problems between |
There was a problem hiding this comment.
I think the more scalable solution might be to just use backticks here and not worry about spaces.
In the assert, we can then replace any whitespace \s+ with a single space ' ' for both the expected and the actual query. See String.prototype.replaceAll()
There was a problem hiding this comment.
the problem is that if you use back ticks then the normal indentation of the code gets added as tabs and it breaks the test. This will probably illustrate the problem better: https://erikmartinjordan.com/avoid-tabs-multiline-string-backticks
Other suggested options would be to use a library, but I thought that might be better solved using native js. https://stackoverflow.com/questions/25924057/multiline-strings-that-dont-break-indentation
There was a problem hiding this comment.
Alternatively we can not indent the code but I don't think that's an ideal solution and it would create lint errors. Also leaving it all in one line and adding \n makes the line too long. I tried all different ways and this seems the most decent solution.
There was a problem hiding this comment.
Given that these are tests, I wouldn't worry too much. If it gets really big, you can move it a file and read that instead IMO
There was a problem hiding this comment.
As I wrote above, we can replace all the whitespace, including any tabs and newlines and such, with a single space before actually asserting the equality. That should ensure that the test still works and still tests that the actual code is equal, right?
There was a problem hiding this comment.
I tried with trim and it didn't work well, will try with replaceAll
There was a problem hiding this comment.
for the record, replaceAll is not supported by all browsers and causes the tests to fail. did the same with replace. https://caniuse.com/?search=replaceAll
There was a problem hiding this comment.
I'm curious to see how replaceAll failed, but I'm fine with just using replace.
Now all that still needs doing is removing this now obsolete commend and then the PR can be approved and merged :)
There was a problem hiding this comment.
It says replaceAll is not a function as the reason for the test to fail. You can try it out. :)
Will delete the comment, thanks for the heads-up.
|
If Michael is happy, I think it's fine to merge this (my virtual approval) |
guergana
force-pushed
the
query-rep-sparql-without-state
branch
from
6302e64 to
c06e65e
Compare
+ added without state to enum + modified QueryRepresentation by adding without state + added test to verify that query is being generated correctly
guergana
force-pushed
the
query-rep-sparql-without-state
branch
from
c06e65e to
bc29ebc
Compare
3 participants
Accidentally worked on this before the ticket was pulled into the sprint. Will leave here for next week