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

REFACTOR: update mirror argument from "position" to "origin" #4714

Closed
wants to merge 5 commits into from

Conversation

marcobiasizzo
Copy link
Contributor

In the mirror function of components_3d and object3d one of the argument is called position. To be coherent with other functions (also with mirror in primitives), the variable name is updated to origin

mirror argument is renamed origin to be coherent with other functions
argument of mirror function is renamed origin to be coherent with other functions
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Thanks @marcobiasizzo for these changes !

We are smoothly transitioning from deprecated arguments into new ones and your proposition seems to align with that effort of standardization !
Do you mind reworking your changes to use our function handler ? This will notify users of the method signature changes and warn them that it will be discarded in the coming weeks/months.

For example, with the mirror method, you would have

    @pyaedt_function_handler(position="origin")
    def mirror(self, position, vector):

You can have a look at duplicate_and_mirror in the same file.

updated also function handler
updated also function handler of mirror
@marcobiasizzo
Copy link
Contributor Author

Sure, thanks! I updated the function handler.
Is it ok now?

@marcobiasizzo
Copy link
Contributor Author

Oh, sorry maybe you meant to cancel my changes and use only the function handler. Let me know!

@maxcapodi78
Copy link
Collaborator

Oh, sorry maybe you meant to cancel my changes and use only the function handler. Let me know!

@marcobiasizzo that's sound good now. Let's see if all UT passes

@maxcapodi78 maxcapodi78 changed the title update mirror argument from "position" to "origin" FIX: update mirror argument from "position" to "origin" May 22, 2024
@maxcapodi78 maxcapodi78 changed the title FIX: update mirror argument from "position" to "origin" REFACTOR: update mirror argument from "position" to "origin" May 22, 2024
@SMoraisAnsys
Copy link
Collaborator

Oh, sorry maybe you meant to cancel my changes and use only the function handler. Let me know!

No, you did exactly what I had in mind, thanks !
Just so you know, our CICD requires access to github secrets (to run the tests on self hosted runners) and, since you are not part of ansys, you can't access the secrets and the tests can't run. I'll Create a new PR with your changes (cherry-picked) that way you'll stay the author of the changes and test will be available to run.

@SMoraisAnsys
Copy link
Collaborator

Closing this PR for its duplicate #4715 where tests should be running.

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.

4 participants