Skip to content

Conversation

@MikeJanes
Copy link
Contributor

@MikeJanes MikeJanes commented Jan 21, 2025

Description

Add ability to get and set persistent id's on objects during stride import and export

Issue linked

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate unit tests.
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved to the PR if any.
  • I have assigned this PR to myself.
  • I have added the minimum version decorator to any new backend method implemented.
  • I have made sure that the title of my PR follows Conventional commits style (e.g. feat: extrude circle to cylinder)

@MikeJanes MikeJanes requested a review from a team as a code owner January 21, 2025 17:10
@MikeJanes MikeJanes requested a review from jonahrb January 21, 2025 17:10
@github-actions github-actions bot added the enhancement New features or code improvements label Jan 21, 2025
@MikeJanes MikeJanes changed the title Enable get/set persistent ids for stride import/export feat: enable get/set persistent ids for stride import/export Jan 21, 2025
@MikeJanes MikeJanes enabled auto-merge (squash) January 21, 2025 20:17
@MikeJanes MikeJanes merged commit 0e37d41 into blitz Jan 21, 2025
20 of 23 checks passed
@MikeJanes MikeJanes deleted the feat/stride_import_id branch January 21, 2025 20:36
@@ -0,0 +1,268 @@
# Copyright (C) 2023 - 2025 ANSYS, Inc. and/or its affiliates.
Copy link
Member

Choose a reason for hiding this comment

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

This entire module shouldn't be here... It should go inside the tools subpackage IMO.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Let's hold off merging if possible next time until I have had a look to the PRs. There are some changes requested on my side for this module. Let's handle them when possible @MikeJanes.

Also, the docstrings are not in-line with what the rest of the library is performing. I am talking mostly about the ones inside the unsupported module. Let's revisit them.

The min_backend_version decorator has been overused as well. All private methods "__" shouldn't have them, because you should have checked it in the calling method already (causing an unnecessary reduction in performance).

Comment on lines +187 to +192
# requires a change to core service, uncomment on next core service update
# assert base_body.id in [b.id for b in bodies1]
# assert wheel_body.id in [b.id for b in bodies2]
assert base_body.id not in [b.id for b in bodies1]
assert wheel_body.id not in [b.id for b in bodies2]

Copy link
Member

Choose a reason for hiding this comment

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

This is going to prevent the update of the Docker images unless we do it right away.... we are checking that the new images are not breaking the tests before promoting them. This is done on main rather than on blitz. And we are safe because of that but nonetheless, it is going to be problematic on the workflows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New features or code improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants