feat: repair tools refactoring #1912
Conversation
umutsoysalansys
changed the title
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1912 +/- ##
==========================================
- Coverage 90.36% 90.34% -0.03%
==========================================
Files 117 120 +3
Lines 9593 9768 +175
==========================================
+ Hits 8669 8825 +156
- Misses 924 943 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Requesting changes. Overall it is looking good but there are some conceptual changes that need to be done:
- All the methods must return a dictionary, with no gRPC objects... If a conversion from a gRPC object needs to be done, we add it to the conversion module. See the work done in feat: grpc driving dimensions stub implementation #1921
- We have to add the noqa statements and the typehints -- otherwise pre-commit will complain
Based on these two comments, the src/ansys/geometry/core/tools/repair_tools.py module will have to be adapted too.
| from ansys.api.geometry.v0.repairtools_pb2 import FindSplitEdgesRequest | ||
|
|
||
| request = FindSplitEdgesRequest( | ||
| bodies_or_faces=kwargs["bodies_or_faces"], | ||
| angle=kwargs["angle"], | ||
| distance=kwargs["distance"], | ||
| ) | ||
| return self.stub.FindSplitEdges(request) |
There was a problem hiding this comment.
I know it might sound silly but, if you look at the other modules with the specific implementations, all of them have separate in-line comments for identifying the sections of the method:
@protect_grpc
def get_collision(self, **kwargs) -> dict: # noqa: D102
from ansys.api.geometry.v0.bodies_pb2 import GetCollisionRequest
# Create the request - assumes all inputs are valid and of the proper type
request = GetCollisionRequest(
body_1_id=kwargs["id"],
body_2_id=kwargs["other_id"],
)
# Call the gRPC service
resp = self.stub.GetCollision(request=request)
# Return the response - formatted as a dictionary
return {"collision_type": resp.collision}Try to follow the same structure and put the same comments. Although repetitive, gives clarity
| from ansys.api.geometry.v0.repairtools_pb2 import FindInexactEdgesRequest | ||
|
|
||
| request = FindInexactEdgesRequest(selection=kwargs["selection"]) | ||
| return self.stub.FindInexactEdges(request) |
There was a problem hiding this comment.
We always have to return a dictionary. It's the base rule for the entire gRPC rearchitecture.
If you need to return an object, you can add a conversion method from a given gRPC response. Please ask @jacobrkerstetter on guidance for this. He has already handled a few of them.
| selection=kwargs["selection"], | ||
| max_edge_length=DoubleValue(value=kwargs["length"]), | ||
| ) | ||
| return self.stub.FindShortEdges(request) |
There was a problem hiding this comment.
The response as a dictionary is a general rule for the entire module. Please adapt everywhere.
There was a problem hiding this comment.
so does it mean we are changing the return type of the methods?
isn't it a breaking change?
There was a problem hiding this comment.
We are not changing the return type of the methods that are user facing, so no, it is not a breaking change.
What we are saying is that anything that comes out of the "services" should be a dictionary with proper Python objects. This implies doing the conversion to the wrappers we have or just extracting the info we need (like the success state). See https://github.com/ansys/pyansys-geometry/pull/1921/files for more info
| def __init__(self, channel: grpc.Channel): | ||
| """Initialize the MeasurementToolsService class.""" | ||
| pass # pragma: no cover | ||
|
|
||
| def find_split_edges(self, **kwargs): | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Remember to add the typehints here as well as the noqa statements. Have a look at the other modules implemented
| # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| # SOFTWARE. | ||
| """Module containing the repair tools service implementation (abstraction layer).""" |
There was a problem hiding this comment.
This is not an abstract layer - it is a specific implementation
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
…om/ansys/pyansys-geometry into feat/grpc-common-layer-refactoring
|
@smereu @RobPasMue only missing piece is refactoring this inspect/repair tools. They are kind of too implicit |
Description
Refactoring repair tools to move repair stubs to the central area. See the issue for more details.
Issue linked
#1817
Checklist
feat: extrude circle to cylinder)