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
Allow for passing IFC Schema attributes on API methods #3851
base: v0.7.0
Are you sure you want to change the base?
Conversation
This is super cool, great idea to parse the schema declaration to decorate function signatures. But does this actually work in an IDE (not Jupyter) and does it detect it properly? I noticed some magic that listens for module importing and patches it then. |
…ks well using AST. Still a couple of minor bugs when building their corresponding file.py
This is the PR that started as a live signature patching, and evolved towards a build step using AST after firstly discussing it with Dion and Andrej. Even if we've talked about it during our meeting, let me write down some background for reference, mixed with some sort of MoM. BackgroundThe approach is conceived in an environment in which the Python API calls were fully type hinted. By bypassing the run centralized call, it adds these type hinting back to methods appended on the file object, so that the user maybe knows that the In my head, it made sense to extend this hinting to API calls closely tied to the schema, such as the well-known And if the answer is yes, it starts to make more sense to append API methods to the file object, rather than having different APIs and having to choose. And this brings us to the status of this PR as of now. BalanceSo, just to list some of the benefits, as discussed during the meeting:
And some drawbacks:
Some further pointsIn this PR, there is one centralized Or also a more functional approach with a traditional snake-case function, with decorators handling the listeners, as discussed in #2693 . Any of the aforementioned two approaches would mean equivalence between calling API actions from file-appended methods and from Also, the integration with the Future stepsI know I'm mixing very fundamental topics as from #2693 and #3306 , altogether with an AST-based build step, but I feel all of these are closely related, and choosing one approach in any of this isolated issues does affect the options for the others. I'll try to think of the reusing schema-built code across versions. Maybe it's as "easy" as implementing an Now, with all of it written down, I'm not sure any of this is going to make it through the scheduled release of the 6th of January, @Moult , maybe it's better to attempt for the next. |
I would prefer this approach. It seems simpler and more elegant than adding more classes. Here's a slight variation on @aothms' original suggestion in #2693 that doesn't rely on duplicating the signature. # in api/root/__init__.py
import ifcopenshell.api.root.create_entity as create entity
# in api/root/create_entity.py
@my_decorator_that_injects_run_listeners
@my_decorator_that_hints_about_schema_differences_or_schema_signature_stuff
def create_entity(file, ifc_class="...", ...):
"""The docstring of the create entity stuff..."""
usecase = Usecase()
usecase.settings = { ... }
return usecase.execute() ... in other words, just take the |
This PR includes a
with_schema_attrs
decorator that allows for the following (material.add_layer
example):Here, attributes from
IfcMaterialLayer
will be parsed from the schema (with a version matchingfile
), exceptMaterial
(since it's already the snake casematerial
argument due to its importance), and with a default value of 1.0 forLayerThickness
. Then, aself.schema_attrs()
method is available to retrieve a dict containing only those schema attributes from within the class body.The usecase help then looks as follows (with API patching from #3830). Note that annotations work with aggregations, select types, etc. I used the available methods on schema declarations for that, recursively. Maybe there was some better way, curious to learn about it if there was!
And it works as expected, if no
LayerThickness
is passed, 1.0 will be used. If aDescription
is used, it will be baked into theIfcMaterialLayer
, etc.Thoughts on this one? it's related to #2878. I find that when an API call is so closely related to the schema, it makes sense to also have the official pascal case attributes available.
As #3830, it relies on signatures and would need to evolve into a more static approach in the future. However, in this case, there are several schemas... I can only think of making file subclasses for each schema, and cmake-generate the corresponding API methods for each one, in the fashion @aothms doesn't like (ifc4file.material.add_layer...). Not the prettiest solution, I know, I'll keep thinking.
Also, the pipe operator generating
types.UnionType
is available from Python 3.10. Is it really needed to support 3.6 to 3.9 for ifcopenshell.api?