FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives

[DaveTwyman]

Create Page and Select Page added to Circuit Primitives

[ansys-reviewer-bot]

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

@ansys-reviewer-bot review

[codecov]

Codecov Report

Attention: Patch coverage is 87.71930% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.39%. Comparing base (f73c6c9) to head (8b94bd1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5283   +/-   ##
=======================================
  Coverage   84.39%   84.39%           
=======================================
  Files         140      140           
  Lines       58645    58684   +39     
=======================================
+ Hits        49493    49528   +35     
- Misses       9152     9156    +4     

[SMoraisAnsys]

Thanks for the changes, could you address the comments and add tests ?

[gmalinve]

@DaveTwyman please install pre-commit in your venv because I see that every time you push a commit it automatically triggers the auto fixes pre-commit:

This package will automatically run all the checks specified in .pre-commit-config.yaml when you push a commit.

Also remember to update and merge main in your branch from time to time.

[DaveTwyman]

@DaveTwyman please install pre-commit in your venv because I see that every time you push a commit it automatically triggers the auto fixes pre-commit: This package will automatically run all the checks specified in .pre-commit-config.yaml when you push a commit.

Also remember to update and merge main in your branch from time to time.

@gmalinve Good point about the pre-commit, I see it's installed on my machine. I've just updated it too. I guess the issue is I need to start using it before each push.

I believe I'm updating main before each push from PyCharm, but let me know if you can see evidence something is wrong here. Thanks again as always

[SMoraisAnsys]

@DaveTwyman If it is installed on your machine then it shouldn't let you commit. The precommit should trigger the problems locally before allowing you to make any commit.

[DaveTwyman]

@DaveTwyman If it is installed on your machine then it shouldn't let you commit. The precommit should trigger the problems locally before allowing you to make any commit.

Ok, understood, I'm just reading up on how to set this up now

[gmalinve]

@DaveTwyman you need to add unit tests now :)

[DaveTwyman]

Could you please use component as variable name instead of comp_name ? This could be confusing for future maintenance as it gives the impression that the variable is a string when it is not. Also can you add a test for create_page ?

Thanks again for taking the time to perform all the requested changes.

@SMoraisAnsys , Not at all, thanks for your patience whilst I work through this.

I could do with some guidance on the unit test for the 'create_page', there is no way to check on the pages name, index or other properties, the underlying IronPython doesn't have such a feature. Are you aware of another instance in PyAEDT where we create something that cannot then be queried. I could then copy that UT. In this case there isn't a useful check that can be added to the UT.

Should we do something like add a dummy UT, that just holds a comment?

[SMoraisAnsys]

I went through Circuit help PDF files and you might find something usefull with SelectPage and GetNumPages.

[DaveTwyman]

I went through Circuit help PDF files and you might find something usefull with SelectPage and GetNumPages.

Yes!, 'GetNumPages', how did I miss that, Thanks

[SMoraisAnsys]

Hi @DaveTwyman, could you please add the last changes required for this PR ? :)

[DaveTwyman]

Yes, I'm planning to get back to finish that shortly

…

________________________________ From: Sébastien Morais ***@***.***> Sent: Tuesday, October 29, 2024 10:45 To: ansys/pyaedt ***@***.***> Cc: David Twyman ***@***.***>; Mention ***@***.***> Subject: Re: [ansys/pyaedt] FEAT: VSin, ISin Sources added to Maxwell Circuit Primitives (PR #5283) [External Sender] Hi @DaveTwyman<https://github.com/DaveTwyman>, could you please add the last changes required for this PR ? :) — Reply to this email directly, view it on GitHub<#5283 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AVVIYTCE36KV4RDR3OZAJQDZ55RNJAVCNFSM6AAAAABPYVZE36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBTHA3DMNBZGQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[DaveTwyman]

Hi @DaveTwyman, could you please add the last changes required for this PR ? :)

Hello @SMoraisAnsys , I've added the last unit test, and method needed to support that test (plus another unit test for the new method). So all the new methods now have supporting unit tests.

It seems that the test coverage codecov is very low, do you think this is because of making multiple single line changes 'id' to 'component' or issues elsewhere?

[SMoraisAnsys]

Try to run your test locally and see if it is correctly covered. It might be a codecov issue (but I doubt about it).
It might be because you are not really going through the lines you expect to go through.
I can't have a look at your PR now and giving general advice so that you can check where the coverage issue comes from.

[gmalinve]

@DaveTwyman please review and fix all the comments.
You created unit tests that don't run at all. Please test, run and debug locally before pushing commits to your PR.