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

feat(standoff)!: return XML alongside HTML for textValue with custom standoff mapping and default XSL transformation (DEV-201) #1991

Merged
merged 56 commits into from Mar 7, 2022

Conversation

BalduinLandolt
Copy link
Collaborator

@BalduinLandolt BalduinLandolt commented Feb 3, 2022

resolves DEV-201

@BalduinLandolt BalduinLandolt self-assigned this Feb 3, 2022
@BalduinLandolt BalduinLandolt changed the title feat(standoff): return XML alongside HTML for textValue with custom mapping and XSLT (DEV-201) feat(standoff)!: return XML alongside HTML for textValue with custom standoff mapping and default XSL transformation (DEV-201) Feb 24, 2022
@BalduinLandolt BalduinLandolt marked this pull request as ready for review Mar 1, 2022
@BalduinLandolt BalduinLandolt requested a review from subotic as a code owner Mar 1, 2022
Copy link
Collaborator

@irinaschubert irinaschubert left a comment

LGTM, just some comments/questions.

docs/01-introduction/standoff-rdf.md Outdated Show resolved Hide resolved
docs/01-introduction/standoff-rdf.md Outdated Show resolved Hide resolved
docs/02-knora-ontologies/knora-base.md Outdated Show resolved Hide resolved
docs/02-knora-ontologies/knora-base.md Show resolved Hide resolved
docs/02-knora-ontologies/knora-base.md Show resolved Hide resolved
sipi/images/0001/.gitignore Show resolved Hide resolved
test_data/ontologies/freetest-onto.ttl Outdated Show resolved Hide resolved
@@ -45,9 +46,13 @@ sealed abstract case class UploadFileRequest private (
case Some(v) => v
case None => FileModelUtil.getDefaultClassName(fileType)
}
val resorceIRIOrNothing = resourceIRI match {
Copy link
Collaborator

@irinaschubert irinaschubert Mar 1, 2022

Choose a reason for hiding this comment

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

Wouldn't maybeResourceIri better match our naming pattern or is there a reason why you used ...orNothing?

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Mar 1, 2022

Choose a reason for hiding this comment

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

I hate those maybeXY variable names. But if you feel it's important to be consistent with this, I can change it. And same goes for the acronyms: I much prefer IRI over Iri but I can change it, if you think the inconsistency hurts too badly.

Copy link
Collaborator

@irinaschubert irinaschubert Mar 1, 2022

Choose a reason for hiding this comment

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

I prefer consistency for both cases. Because consistency let's us recognize similar constructs (which helps understanding code written by someone else) and also makes searching for certain constructs easier (although right now I don't know why I would want to search for all maybe... constructs - but you never know ;). This is just my opinion, maybe the others don't agree.

Copy link
Collaborator

@mpro7 mpro7 Mar 2, 2022

Choose a reason for hiding this comment

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

@BalduinLandolt why is like that? maybe prefix is very descriptive and suggests what actually is stored in that named property.
We already agreed some time ago to use capital letters for acronyms in property names, so this would be maybeResourceIRI.

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Mar 3, 2022

Choose a reason for hiding this comment

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

I think the reason why I don't like "maybe" is because it's added at the beginning, so two variables don't look similar:

val something = "foo bar"
val maybeSomething = Some(something) // not clear on first glance that it's the same
val somethingOption = Some(something) // more clear on first glance that it's the same

Does that make sense? But I'm absolutely OK with "maybe" if you think it's good.

However, in this case it's not actually correct to call it maybeResourceIRI because it's not an option, it's either a string or an empty string, depending on the input. I'll rename it to resourceIRIOrEmptyString

Copy link
Collaborator

@mpro7 mpro7 Mar 3, 2022

Choose a reason for hiding this comment

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

Yeah that is the downside. From the other hand, if we use typing everywhere, we could skip naming properties like that, because type annotation strictly say what is this. somethingMaybe aka callMeMaybe ;) could be also another idea.

Copy link
Collaborator

@irinaschubert irinaschubert Mar 3, 2022

Choose a reason for hiding this comment

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

I see your point. Still, it is the naming pattern we have right now and in my opinion it makes it easier to understand the code if the pattern is the same everywhere. We could still introduce a new naming pattern in V3. As the value here is not an option, you are actually right that it doesn't have to match the naming pattern of options. So, IMO you can as well call it resourceIRIOrEmptyString.

Copy link
Collaborator

@subotic subotic Mar 7, 2022

Choose a reason for hiding this comment

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

please don't change patterns in a codebase that uses this pattern all over the place. Either it is changed everywhere or keep the to the current convention. conventions need to be carefully analysed and then when agreed upon used by everybody. you can add it to the list of changes that you would like to see in the new codebase.

mpro7
mpro7 approved these changes Mar 1, 2022
Copy link
Collaborator

@mpro7 mpro7 left a comment

Just found minor issues.

docs/02-knora-ontologies/knora-base.md Show resolved Hide resolved
docs/03-apis/api-v2/editing-values.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/reading-and-searching-resources.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/reading-and-searching-resources.md Outdated Show resolved Hide resolved
sipi/images/0001/Cpl5d73kOLz-FclZg2VVf6r.info Show resolved Hide resolved
sipi/images/0801/.gitignore Show resolved Hide resolved
subotic
subotic approved these changes Mar 3, 2022
Copy link
Collaborator

@subotic subotic left a comment

LGTM

webapi/src/test/scala/org/knora/webapi/E2ESpec.scala Outdated Show resolved Hide resolved
@sonarcloud
Copy link

@sonarcloud sonarcloud bot commented Mar 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@BalduinLandolt BalduinLandolt merged commit 2548b8f into main Mar 7, 2022
11 checks passed
@BalduinLandolt BalduinLandolt deleted the wip/DEV-201-return-custom-standoff-as-xml branch Mar 7, 2022
@daschbot daschbot mentioned this pull request Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants