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(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

Conversation

@subotic
Copy link
Collaborator

@subotic subotic commented Jun 5, 2021

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 Jun 15, 2021
@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 added 2 commits Jul 12, 2021
# Conflicts:
#	webapi/src/main/scala/org/knora/webapi/responders/Responder.scala
@subotic
Copy link
Collaborator Author

@subotic 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 SepidehAlassi commented Jul 14, 2021

@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 subotic marked this pull request as ready for review Aug 6, 2021
Copy link
Contributor

@BalduinLandolt BalduinLandolt left a comment

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.

@subotic
Copy link
Collaborator Author

@subotic subotic commented Aug 11, 2021

@BalduinLandolt @irinaschubert thanks for the review.

@subotic
Copy link
Collaborator Author

@subotic 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
11 checks passed
@subotic subotic deleted the wip/DSP-1700-Remove-cardinalities-from-class branch Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants