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.

Sign up for GitHub

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

Jump to bottom

feat: setting instance name during component creation #1382

Merged
merged 22 commits into from
Sep 5, 2024
Merged

feat: setting instance name during component creation #1382

merged 22 commits into from
Sep 5, 2024

Conversation

umutsoysalansys
Copy link
Contributor

@umutsoysalansys umutsoysalansys commented Aug 28, 2024 •
edited
Loading

Description

**This change enables users to set the instance name (optional) while creating a component. **
image

Issue linked

Please mention the issue number or describe the problem this pull request addresses.

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)

@github-actions github-actions bot added the enhancement New features or code improvements label Aug 28, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 28, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.80%. Comparing base (e46854e) to head (ad711b2).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1382   +/-   ##
=======================================
  Coverage   91.79%   91.80%           
=======================================
  Files          86       86           
  Lines        6824     6831    +7     
=======================================
+ Hits         6264     6271    +7     
  Misses        560      560

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

@umutsoysalansys umutsoysalansys marked this pull request as ready for review September 2, 2024 13:19
@umutsoysalansys umutsoysalansys requested a review from a team as a code owner September 2, 2024 13:19
@umutsoysalansys
Copy link
Contributor Author

@b-matteo @RobPasMue ready for review:)

RobPasMue
RobPasMue requested changes Sep 3, 2024
View reviewed changes
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.

Several cleanup comments and improvements. Also, @umutsoysalansys - have you checked with @rajesh1359 or @agvarghe that this is what they were expecting? I'm not sure where the request for this is coming from

src/ansys/geometry/core/designer/component.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/component.py Show resolved Hide resolved
src/ansys/geometry/core/designer/component.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/component.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/component.py Outdated Show resolved Hide resolved
tests/integration/test_design.py Outdated Show resolved Hide resolved
tests/integration/test_design.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/designer/component.py Show resolved Hide resolved
@RobPasMue
Copy link
Member

Please update your branch with respect to main as well.

umutsoysalansys and others added 7 commits September 3, 2024 08:28
@umutsoysalansys @RobPasMue
Update src/ansys/geometry/core/designer/component.py
33c564d 
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@umutsoysalansys @RobPasMue
Update src/ansys/geometry/core/designer/component.py
1f969b8 
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@umutsoysalansys @RobPasMue
Update src/ansys/geometry/core/designer/component.py
41f69e7 
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@umutsoysalansys @RobPasMue
Update tests/integration/test_design.py
ed7dda8 
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@umutsoysalansys @RobPasMue
Update tests/integration/test_design.py
89c25b7 
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@umutsoysalansys
typos
0f72375
@umutsoysalansys
Merge branch 'main' into feat/component_instance_name
2f8b908
RobPasMue
RobPasMue requested changes Sep 3, 2024
View reviewed changes
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.

If you are resolving comments, make sure you are either answering them or committing my suggestions. Do not just close them @umutsoysalansys

src/ansys/geometry/core/designer/component.py Show resolved Hide resolved
@RobPasMue
Copy link
Member

Windows new version is working. Linux version is having issues. @b-matteo investigating.

@umutsoysalansys
Copy link
Contributor Author

@RobPasMue, updated API Server, linux tests are passing. What is this testing with minimum requirement workflow? @b-matteo How to test locally?

@umutsoysalansys
Copy link
Contributor Author

@RobPasMue pls review and approve thanks.

@RobPasMue
Copy link
Member

@RobPasMue, updated API Server, linux tests are passing. What is this testing with minimum requirement workflow? @b-matteo How to test locally?

Minimum requirement workflow just tries to install the project without the "test-pinned" dependencies. It runs on all supported Python versions, but it also uses the linux-latest image. Since we have not pushed the unstable ones to stable yet, the tests related to the instance_name. Don't worry @umutsoysalansys - once we push the images to stable, the checks will pass.

@umutsoysalansys
Copy link
Contributor Author

umutsoysalansys commented Sep 5, 2024 via email

@RobPasMue
Copy link
Member

@RobPasMue pls review and approve thanks.

You are not the only one wanting to finish this PR @umutsoysalansys - we are all waiting for things to work properly and pass in the workflow. Until then, we cannot merge. You can assume it's approved anyway.

@umutsoysalansys
Copy link
Contributor Author

umutsoysalansys commented Sep 5, 2024 •
edited by RobPasMue
Loading

what is holding it back? Unstable images are passing? @RobPasMue

@RobPasMue
Copy link
Member

what is holding it back? Unstable images are passing? @RobPasMue

Linux latest unstable image is updated. ADO is still pushing the Windows image.

@umutsoysalansys
Copy link
Contributor Author

umutsoysalansys commented Sep 5, 2024 •
edited
Loading

what is holding it back? Unstable images are passing? @RobPasMue

Linux latest unstable image is updated. ADO is still pushing the Windows image.

Ready to approve and merge @RobPasMue

@umutsoysalansys
Copy link
Contributor Author

maybe we need an reviewer/approver in US time zone. so we can move faster.

@RobPasMue
Copy link
Member

@umutsoysalansys please stop, we have already heard you

RobPasMue
RobPasMue approved these changes Sep 5, 2024
View reviewed changes
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.

LGTM - merging

@RobPasMue RobPasMue merged commit 7039aea into main Sep 5, 2024
43 checks passed
@RobPasMue RobPasMue deleted the feat/component_instance_name branch September 5, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobPasMue RobPasMue RobPasMue approved these changes

@b-matteo b-matteo Awaiting requested review from b-matteo

@smereu smereu Awaiting requested review from smereu

Assignees

@umutsoysalansys umutsoysalansys

Labels
enhancement New features or code improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@umutsoysalansys @RobPasMue @smereu @b-matteo @pyansys-ci-bot