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

Update 223P to public advisory review version #309

Open
wants to merge 162 commits into
base: develop
Choose a base branch
from
Open

Conversation

gtfierro
Copy link
Collaborator

@gtfierro gtfierro commented Mar 20, 2024

There's a whole mess of changes in here, sorry! I'll summarize them here:

  • miscellaneous updates of dependencies for bug fixes
  • integration tests just use the topquadrant engine for now (much faster than the other engine pyshacl; working out how we can still test the pyshacl stuff)
  • add support for the shacl engine in the app API
  • incorporate flag for not doing shacl validation on a library (speeds up loading during tests)
  • incorporate flag for speeding up library loading by not doing template inference, and pushing this configuration flag and the prior one
  • updating the 223P ontology! adding ontology dependencies too

TODO:

  • check all tests run
  • why are there dependencies commented out in one of the template files?

gtfierro and others added 30 commits January 19, 2023 08:52
@gtfierro
Copy link
Collaborator Author

gtfierro commented Jul 5, 2024

@TShapinsky I think this is finally ready for review!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
buildingmotif/api/app.py Show resolved Hide resolved

# non-singleton BuildingMOTIF
class BuildingMOTIF:
"""Manages BuildingMOTIF data classes."""
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be avoided by the following:
You can create multiple instances of BuildingMOTIF if you call BuildingMOTIF.clean() between each constructor call. This only deletes the current instance from the class object, you can still keep it and do stuff with 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.

!!! Awesome

Copy link
Member

Choose a reason for hiding this comment

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

One additional thing I just thought of, you may be able to avoid the monkeypatch stuff for get_building_motif by just setting the instance attribute on the BuildingMOTIF class to the one you want to use.

@TShapinsky
Copy link
Member

Looks good, @gtfierro. I have a couple more comments, but hopefully that should be it.

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

3 participants