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!: add isSequenceOf to knora-base ontology (DEV-745) #2061

Merged
merged 59 commits into from Aug 3, 2022

Conversation

BalduinLandolt
Copy link
Collaborator

@BalduinLandolt BalduinLandolt commented May 9, 2022

Resolves DEV-745

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other: Changes to the knora-base onrtology

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, what isPartOf is for images.

Does this PR introduce a breaking change?

  • Yes
  • No

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

@BalduinLandolt BalduinLandolt self-assigned this May 9, 2022
@BalduinLandolt BalduinLandolt changed the title feat: add isSequenceOf to knora-base ontology (DEV-745) feat!: add isSequenceOf to knora-base ontology (DEV-745) May 9, 2022
@sonarcloud
Copy link

sonarcloud bot commented May 30, 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

Copy link

@irinaschubert irinaschubert left a 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.

docs/02-knora-ontologies/knora-base.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 Outdated Show resolved Hide resolved
Copy link

@irinaschubert irinaschubert left a 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?

* @param value the string value of the LangString.
* @return a LanguageCode value object
*/
def unsafeMake(language: LanguageCode, value: String): LangString =

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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) {}

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.

Copy link
Collaborator Author

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.

Comment on lines 40 to 50
/**
* 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) {}
}
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Comment on lines 55 to 71
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) {}
}
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@mpro7
Copy link
Collaborator

mpro7 commented Jul 26, 2022

Forgot to ask actually out of the curiosity, why the LangString is implemented in this PR?

@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 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 9 Code Smells

No Coverage information No Coverage information
23.0% 23.0% Duplication

@BalduinLandolt BalduinLandolt merged commit 74366d4 into main Aug 3, 2022
@BalduinLandolt BalduinLandolt deleted the wip/DEV-745-add-issequenceof branch August 3, 2022 11:07
@daschbot daschbot mentioned this pull request Aug 3, 2022
@BalduinLandolt BalduinLandolt restored the wip/DEV-745-add-issequenceof branch August 3, 2022 11:57
@BalduinLandolt BalduinLandolt deleted the wip/DEV-745-add-issequenceof branch August 4, 2022 07:13
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

4 participants