Skip to content

101 domain model control over asset palette categorization #221

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

Merged

Conversation

kenmeacham
Copy link
Contributor

No description provided.

@kenmeacham kenmeacham linked an issue Nov 6, 2024 that may be closed by this pull request
@kenmeacham kenmeacham requested a review from mike1813 November 7, 2024 11:18
@kenmeacham
Copy link
Contributor Author

@mike1813 - I've put you as reviewer for this pull request, as you understand it the best. In the end, it is only a minor fix to the SPARQL query, an addition to the core model and a fix to the unit test. See issue #101 for screenshots of how it looks now, so no need to spin it up yourself. See also a question about the fix to the core model.

Copy link
Member

@mike1813 mike1813 left a comment

Choose a reason for hiding this comment

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

@kenmeacham : I suggest two small changes before we merge this:

  • Make core#PaletteType a subclass of #AtomicThing, which is the base class for core#Asset.
  • Delete the lines that add typeLabel in the report generator, rather than commenting them.

The first change is just so core#PaletteType looks as similar to core#Asset as possible. The properties are the same for #CompositeThing and #AtomicThing so it should work as it is. The only reason to switch to #AtomicThing is so it will still work if we ever need to switch from core#Asset to core#PaletteType in some other query.

The second change is just to avoid retaining old code we don't really need. It doesn't look like we'll ever want to reinstate the commented lines, so keeping them in just detracts from the code readability.

Copy link

@kenmeacham
Copy link
Contributor Author

@kenmeacham : I suggest two small changes before we merge this:

* Make core#PaletteType a subclass of #AtomicThing, which is the base class for core#Asset.

* Delete the lines that add typeLabel in the report generator, rather than commenting them.

The first change is just so core#PaletteType looks as similar to core#Asset as possible. The properties are the same for #CompositeThing and #AtomicThing so it should work as it is. The only reason to switch to #AtomicThing is so it will still work if we ever need to switch from core#Asset to core#PaletteType in some other query.

The second change is just to avoid retaining old code we don't really need. It doesn't look like we'll ever want to reinstate the commented lines, so keeping them in just detracts from the code readability.

OK. I've made the suggested changes. All seems to still work as expected. You should be able to just approve this now, so I can do the merge.

@kenmeacham kenmeacham requested a review from mike1813 November 18, 2024 09:45
Copy link
Member

@mike1813 mike1813 left a comment

Choose a reason for hiding this comment

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

Looks good.

@kenmeacham kenmeacham merged commit 88f1ee1 into dev Nov 20, 2024
3 checks passed
@kenmeacham kenmeacham deleted the 101-domain-model-control-over-asset-palette-categorization branch November 20, 2024 16:10
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.

Domain model control over asset palette categorization
2 participants