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

Accept a Python range when setting scoping IDs. #2001

Merged
merged 13 commits into from
Jan 10, 2025

Conversation

PProfizi
Copy link
Contributor

@PProfizi PProfizi commented Jan 10, 2025

Hi @JennaPaikowsky, the PR for the field inspired me to do this for the Scoping class as I noticed we were doing list(range(*)) in the docstring examples for the Field when defining a Scoping.

This PR mainly adds the support for a range as input when setting the IDs of a scoping.

It also adds type hinting and improves the docstrings a bit. I'll also add some docstring examples and testing.

I'll actually add the AnyServerType type for type hint in a dedicated PR if @moe-ad and @jorgepiloto confirm this is the correct way to provide a combination of allowed types for type hints in the rest of the library. See #2002

@jorgepiloto @moe-ad one thing I would like our opinion on is the type hint for pointers and Grpc messages as I think it will not work as-is. Edit: it looks like it does! Even though the IDE is complaining that ansys.grpc.dpf.scoping_pb2.Scoping does not exist.

@PProfizi PProfizi added the enhancement New feature or request label Jan 10, 2025
@PProfizi PProfizi self-assigned this Jan 10, 2025
@PProfizi PProfizi requested a review from jorgepiloto January 10, 2025 11:27
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.38%. Comparing base (52ed660) to head (72b1a43).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2001      +/-   ##
==========================================
- Coverage   88.40%   88.38%   -0.02%     
==========================================
  Files          89       89              
  Lines       10252    10261       +9     
==========================================
+ Hits         9063     9069       +6     
- Misses       1189     1192       +3     

@rafacanton
Copy link
Contributor

@PProfizi I see post ci failing, do we need a PR there to support something related to the server? Otherwise, this LGTM!

@PProfizi
Copy link
Contributor Author

Ah, indeed, @rafacanton the scoping factory for named selections has a server parameter which is not used. I removed it but of course this is not backwards compatible with existing calls.

Copy link
Contributor

@JennaPaikowsky JennaPaikowsky left a comment

Choose a reason for hiding this comment

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

A couple of minor points.

@PProfizi PProfizi added this to the 0.13.4 milestone Jan 10, 2025
@PProfizi PProfizi merged commit 59f6708 into master Jan 10, 2025
45 of 46 checks passed
@PProfizi PProfizi deleted the feat/scoping_ids_accept_range branch January 10, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants