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.

Sign up for GitHub

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

Jump to bottom

feat(v2-ontologies): add remove cardinalities from class if property not used in resources (DSP-1700) #1869

Merged
merged 32 commits into from Aug 11, 2021
Merged

feat(v2-ontologies): add remove cardinalities from class if property not used in resources (DSP-1700) #1869

merged 32 commits into from Aug 11, 2021

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Jun 5, 2021 •
edited

resolves DSP-1700

  • start breaking up OntologyResponderV2 into subpackages
  • add "unit" tests for sub-functions
  • add new freetest ontology to anything project
  • add org.knora.webapi.responders.v2.ontology.DeleteCardinalitiesFromClass

@subotic subotic self-assigned this Jun 5, 2021
@kilchenmann kilchenmann changed the title feat (v2-ontologies): improve removing cardinalities from class feat(v2-ontologies): improve removing cardinalities from class Jun 8, 2021
@subotic subotic marked this pull request as draft June 15, 2021 07:14
@kilchenmann kilchenmann changed the title feat(v2-ontologies): improve removing cardinalities from class feat(v2-ontologies): improve removing cardinalities from class (DSP-1700) Jul 3, 2021
@subotic subotic changed the title feat(v2-ontologies): improve removing cardinalities from class (DSP-1700) feat(v2-ontologies): add route to remove cardinalities from class if property is not used in data (DSP-1700) Jul 12, 2021
@subotic
Merge branch 'main' into wip/DSP-1700-Remove-cardinalities-from-class
124aab4 
# Conflicts:
#	webapi/src/main/scala/org/knora/webapi/responders/Responder.scala
@subotic
fix: after merge
3e6ad44
@subotic
Copy link
Collaborator Author

subotic commented Jul 13, 2021

@SepidehAlassi could you maybe take a look? I still need to add some tests, but the new test in OntologyResponderV2Spec seems to be working. Did I miss a check?

@SepidehAlassi
Copy link
Contributor

SepidehAlassi commented Jul 14, 2021 •
edited

@subotic you have changed the architecture of the code, this new one is very messy like Admin api, and it is hard to understand.
In api V2, we have a very consistent style of defining classes and methods preserved on the entire code base of api v2.
Additionally, we have followed rules for clean code for naming classes, methods, and variables. Each variable name indicates its content, and each function has a clear name describing what it does. Lines of documentation are added to complex parts of code to help other developers who might work with the code. Please follow these when working with api v2.

@subotic subotic added the v14 label Jul 22, 2021
@subotic subotic changed the title feat(v2-ontologies): add route to remove cardinalities from class if property is not used in data (DSP-1700) feat(v2-ontologies): add remove cardinalities from class if property is not used in data (DSP-1700) Jul 22, 2021
@subotic subotic changed the title feat(v2-ontologies): add remove cardinalities from class if property is not used in data (DSP-1700) feat(v2-ontologies): add remove cardinalities from class if property not used in data (DSP-1700) Jul 22, 2021
@subotic subotic changed the title feat(v2-ontologies): add remove cardinalities from class if property not used in data (DSP-1700) feat(v2-ontologies): add remove cardinalities from class if property not used in resources (DSP-1700) Jul 22, 2021
@subotic
feat(ontologies): add route to remove cardinalities from class if pro…
92a88be 
…perty is not used in data
@subotic
feat(ontologies): add route to remove cardinalities from class if pro…
dd0ba4c 
…perty is not used in data
@subotic
feat(ontologies): add route to remove cardinalities from class if pro…
e774aed 
…perty is not used in data
@subotic
refactor: fix deprecations
e89cab4
@subotic
test: add regenerated test responses
402966d 
after adding freetest ontology to the spec
@subotic
feat(ontologies): add route to remove cardinalities from class if pro…
c32495f 
…perty is not used in data
@subotic
test(ontologies): add remove cardinalities from class if property is …
a668e8d 
…not used in data
@subotic
test(ontologies): add remove cardinalities from class if property is …
e75deae 
…not used in data
@subotic
Merge branch 'main' into wip/DSP-1700-Remove-cardinalities-from-class
12a5b54
@subotic
refactor: fix deprecations
79a6c09
@subotic subotic marked this pull request as ready for review August 6, 2021 09:55
BalduinLandolt
BalduinLandolt approved these changes Aug 10, 2021
View reviewed changes
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

With the amount of refactoring done in this PR, it's hard to do a sensible code review. But generally it makes sense.

However, I really don't think that changes to .scalafmt.conf should be done in a PR that is supposed to be reviewed. It's really a pain (and a waste of time) to have to dig through thousands of lines of auto-formating-changes, just to find the 50 lines that are relevant to the review.

docs/03-apis/api-v2/ontology-information.md Show resolved Hide resolved
test_data/ontologyR2RV2/anythingOntologyWithValueObjects.ttl Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/app/ApplicationActor.scala Show resolved Hide resolved
.../main/scala/org/knora/webapi/messages/v2/responder/ontologymessages/OntologyMessagesV2.scala Show resolved Hide resolved
.../main/scala/org/knora/webapi/messages/v2/responder/ontologymessages/OntologyMessagesV2.scala Outdated Show resolved Hide resolved
webapi/src/main/twirl/org/knora/webapi/queries/sparql/v2/isPropertyUsed.scala.txt Show resolved Hide resolved
.../main/scala/org/knora/webapi/messages/v2/responder/ontologymessages/OntologyMessagesV2.scala Outdated Show resolved Hide resolved
@kilchenmann kilchenmann mentioned this pull request Aug 10, 2021
Merged
@subotic
Copy link
Collaborator Author

subotic commented Aug 11, 2021

@BalduinLandolt @irinaschubert thanks for the review.

@subotic
Copy link
Collaborator Author

subotic commented Aug 11, 2021

@irinaschubert I will merge this PR after the tests pass.

@subotic subotic merged commit a30668b into main Aug 11, 2021
@subotic subotic deleted the wip/DSP-1700-Remove-cardinalities-from-class branch August 11, 2021 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kilchenmann kilchenmann kilchenmann left review comments

@SepidehAlassi SepidehAlassi SepidehAlassi left review comments

@irinaschubert irinaschubert irinaschubert approved these changes

@BalduinLandolt BalduinLandolt BalduinLandolt approved these changes

Assignees

@subotic subotic

Labels
v14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@subotic @SepidehAlassi @kilchenmann @irinaschubert @BalduinLandolt