Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement
LiteralOrIri
#1186Implement
LiteralOrIri
#1186Changes from 11 commits
16c6904
66fd51c
bac405d
d12a556
959636f
3524d3a
a8cebea
eca253d
f326fae
8e52000
3c6153d
49d267d
bd32608
e27f682
3b614fe
9507c9d
6ff0ecf
e0372aa
bb8b089
f3ba1db
bd9b819
a234ef7
a075f44
7683bfe
a6d2cc1
766c2fb
8093a4d
0b80900
d3fd6e3
30c2815
5bc1345
8223519
d3e4e97
e072cf4
f74084b
2ca364a
4aef562
0fb6684
72a9673
74797f7
a5d0649
cb5155e
d91739e
f91df95
7d1c9b8
fe42703
fdebba3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The interface should probably be something like
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.
just
getContent()
then it's the same as for theLiteral
class which makes the usage easier.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.
Okay after further thinking:
Maybe we mostly use the parser for the parsing and taking the NormalizedString directly is fine, but then you really really have to document whether this should be with or without the
<>
. And maybe warn if it starts or ends with those (theoretically you can escape them, but what are the odds?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.
So bottomline: It is important to maake it very clear when you get
<>
and when not.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.
Also use `AD_CONTRACT_CHECK as soon as I've merged it with the additional message.
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.
same comment about
AD_CONTRACT_CHECK
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.
Just
getIri
andgetLiteral
but also add theconst& getBla() const;
overloads.
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.
getIriContent()
for consistency.getContent
that gives us the normalized content of either the literal or the IRI.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 am currently working on an implementation of
AD_CONTRACT_CHECK(hasDatatype(), "getDatatype() was called on a
Literalwhich has no datatype")
;(More concise). I will let you know when this is merged.
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.
Also make it always clear whether you get the quotation marks.
I am still not sure the more I am thinking about this, whether we shouldn't doe the parsing directly inside these classes, then we can enforce more invariants about the " @ and <> characters.
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.
Or at least make the constructor private and have only certain authorized parsers create such objects.
But we can discuss this at a later point.
Check warning on line 12 in src/parser/NormalizedString.cpp
Codecov / codecov/patch
src/parser/NormalizedString.cpp#L9-L12
Check warning on line 24 in src/parser/NormalizedString.cpp
Codecov / codecov/patch
src/parser/NormalizedString.cpp#L24
Check warning on line 27 in src/parser/NormalizedString.cpp
Codecov / codecov/patch
src/parser/NormalizedString.cpp#L26-L27
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 function is also
unsafe
in some way.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.
There are various different normalizations, maybe we should talk about this in person on how to best do this.