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

Generalize edge property type implementation #607

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

greschd
Copy link
Member

@greschd greschd commented Oct 3, 2024

Stores the linked object within the protobuf message (w/ resource path) + keeps its server wrapper for instantiating. The previous implementation directly stored the linked object, but that's more difficult to generalize w.r.t. _to_pb_object / _from_pb_object.

TODO:

  • apply to all edge property types and fix any upcoming issues.
  • enable allowed_types to be dependent on the parent type (needed for LinkedSelectionRule)
  • currently needs the ability to initialize without any arguments - ok, or does this need to change?
  • do we need to create a copy in _to_pb_object, or is it ok to return the internal object directly?

Allow marking gRPC properties as supported since a specific
server version. The `grpc_data_property_read_only` is given
a `supported_since` keyword, and `grpc_data_property` is given
two separate keywords `readable_since` and `writable_since`.

Other changes:
- Change the `xfail_before` test fixture to `raises_before_version`,
  which explicitly checks that a `RuntimeError` is raised when run
  on an older server version.
- Move the `supported_since` implementation to a separate file.
- In the CI definition, reuse the `DOCKER_IMAGE_NAME` variable
  in more places.
Add a '_SUPPORTED_SINCE' class attribute to the 'GrpcObjectBase'
class, which has two effects:
- upon storing an object, check the server version and raise an
  appropriate exception if the server version is too low
- in the 'mark_grpc_properties' class decorator, add a line at the
  end of the class docstring indicating the server version at which
  the class was introduced
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 85.39326% with 13 lines in your changes missing coverage. Please review.

Project coverage is 93.11%. Comparing base (9853384) to head (9d8abd2).

Files with missing lines Patch % Lines
.../_tree_objects/_grpc_helpers/edge_property_list.py 82.66% 13 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                         @@
##           feat/582-add-buttjointsequence     #607      +/-   ##
==================================================================
- Coverage                           93.19%   93.11%   -0.08%     
==================================================================
  Files                                  86       86              
  Lines                                4583     4619      +36     
==================================================================
+ Hits                                 4271     4301      +30     
- Misses                                312      318       +6     
Flag Coverage Δ
python-3.10 93.11% <85.39%> (-0.08%) ⬇️
python-3.11 93.02% <85.39%> (-0.08%) ⬇️
python-3.12 93.02% <85.39%> (-0.08%) ⬇️
server-latest 93.11% <85.39%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@greschd greschd force-pushed the feat/582-add-buttjointsequence branch from 9853384 to 84cd4de Compare October 3, 2024 16:36
@greschd greschd force-pushed the feat/582-add-buttjointsequence branch 3 times, most recently from ee05fe5 to 774123c Compare October 23, 2024 12:29
Base automatically changed from feat/582-add-buttjointsequence to main October 23, 2024 12:41
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.

1 participant