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!: add isSequenceOf to knora-base ontology (DEV-745) #2061
Conversation
Kudos, SonarCloud Quality Gate passed!
|
…sources in ontology
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 isSequenceOf part looks good to me.
Regarding the value objects, I would add unit tests for all value objects you introduced. Also, I think it would be good to introduce value objects for Class/PropertyLabel and Class/PropertyComment.
webapi/src/main/scala/org/knora/webapi/routing/v2/OntologiesRouteV2.scala
Outdated
Show resolved
Hide resolved
dsp-schema/core/src/main/scala/dsp/schema/domain/SchemaCommands.scala
Outdated
Show resolved
Hide resolved
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 new value object of the request payload kind of looks messy with the SmartIri warnings. Now, I'm not sure anymore if it makes sense to have value objects for payloads at all. Maybe we can this discuss this in detail in our API meeting?
dsp-schema/core/src/test/scala/dsp/schema/domain/SchemaCommandsSpec.scala
Outdated
Show resolved
Hide resolved
dsp-schema/core/src/test/scala/dsp/schema/domain/SchemaCommandsSpec.scala
Outdated
Show resolved
Hide resolved
* @param value the string value of the LangString. | ||
* @return a LanguageCode value object | ||
*/ | ||
def unsafeMake(language: LanguageCode, value: String): LangString = |
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.
Do you actually use this somewhere? If not, I would not implement it.
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.
you're right, it's not used anymore. I'll delete it
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.
actually, no, I use it after all... I put it on the "to discuss" list.
Validation.succeed(new LanguageCode(value) {}) | ||
} | ||
|
||
lazy val en: LanguageCode = new LanguageCode(V2.EN) {} |
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.
Can the language codes from V2 be moved to the right place? I think the language codes could even live inside this file (.../valueobjects/LangString.scala) but I am not 100% sure if this is the best location.
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.
you're right. But I'll wait with that for the thursday meeting.
dsp-shared/src/test/scala/dsp/valueobjects/LangStringSpec.scala
Outdated
Show resolved
Hide resolved
dsp-schema/core/src/test/scala/dsp/schema/domain/SchemaCommandsSpec.scala
Outdated
Show resolved
Hide resolved
/** | ||
* Unsafely creates a LangString value object. | ||
* Warning: skipps all validation. Should not be used unless there is no possibility for the data to be invalid. | ||
* | ||
* @param languagea [[LanguageCode]] value object representing the language of the LangString. | ||
* @param value the string value of the LangString. | ||
* @return a LanguageCode value object | ||
*/ | ||
def unsafeMake(language: LanguageCode, value: String): LangString = | ||
new LangString(language, value) {} | ||
} |
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.
Why is this actually for? I think we shouldn't make things like that possible.
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.
It was an experiment inbetween, and proved to be actually quite nice to have. But in the end I could get rid of it, so it's not in use anymore, and I'll delete it.
But I could imagine we'll need it in the future after all... we'll have to see.
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.
Actually, no, I just got an error because I use it after all. Let's discuss it in the meeting tomorrow too.
sealed abstract case class LanguageCode private (value: String) | ||
object LanguageCode { self => | ||
def make(value: String): Validation[ValidationException, LanguageCode] = | ||
if (value.isEmpty) { | ||
Validation.fail(ValidationException(LanguageCodeErrorMessages.LanguageCodeMissing)) | ||
} else if (!V2.SupportedLanguageCodes.contains(value)) { | ||
Validation.fail(ValidationException(LanguageCodeErrorMessages.LanguageCodeInvalid)) | ||
} else { | ||
Validation.succeed(new LanguageCode(value) {}) | ||
} | ||
|
||
lazy val en: LanguageCode = new LanguageCode(V2.EN) {} | ||
lazy val de: LanguageCode = new LanguageCode(V2.DE) {} | ||
lazy val fr: LanguageCode = new LanguageCode(V2.FR) {} | ||
lazy val it: LanguageCode = new LanguageCode(V2.IT) {} | ||
lazy val rm: LanguageCode = new LanguageCode(V2.RM) {} | ||
} |
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 find this part unnecessarily separated and whole LangString
implementation kind of overcomplicated. How about this? It is from my ongoing branch and some changes are not yet pushed, but IMO looks cleaner.
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.
let's discuss this in the thursday meeting. I think we need a value object for LanguageCode, but it's of course up for discussion
webapi/src/test/scala/org/knora/webapi/models/OntologyModels.scala
Outdated
Show resolved
Hide resolved
Forgot to ask actually out of the curiosity, why the |
Kudos, SonarCloud Quality Gate passed!
|
Resolves DEV-745
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
For video and audio, there is no analogous construct as
isPartOf
for images.Issue Number: DEV-745
What is the new behavior?
isSequenceOf
is added, which is for video and audio, whatisPartOf
is for images.Does this PR introduce a breaking change?
This PR requires existing databases to reload the knora-base ontology, for which an upgrade script is needed, which requires a new major version of the ontology.
Other information