Skip to content

Fix return instead of raise. #1332

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

Merged
merged 4 commits into from
Jul 11, 2025
Merged

Conversation

benoit-cty
Copy link
Contributor

@benoit-cty benoit-cty commented Jul 11, 2025

Technical changes

  • the modify_my_parameters function should return the ParameterNode, but you're not getting the expected error. In the modify_parameters method on line 81, there's a bug in the code. It should raise a ValueError, but instead it returns a ValueError. This means the error is created but not actually thrown, so your code continues executing without the expected error.

  • Will close Error not raised when building an erroneous reform #1331

@MattiSG
Copy link
Member

MattiSG commented Jul 11, 2025

@benoit-cty that's a lot of added tests to review, and some checks fail. Could you please clarify if you wrote these tests yourself, or if they were generated by an LLM? If so, which review mechanism did you put in place to ensure that all these tests are relevant and warrant another human to review them? Thank you!

@benjello
Copy link
Member

@MattiSG @benoit-cty : this PR fix a recurrent annoying error difficult to grasp.
I think tests are welcom and not to numerous.
However the failing tests are related to retiring windows-server which is a separate issue.

@benjello benjello self-requested a review July 11, 2025 09:28
@benoit-cty benoit-cty merged commit d0b144c into master Jul 11, 2025
23 checks passed
@benoit-cty benoit-cty deleted the fix/modify_parameters_raise_ValueError branch July 11, 2025 13:14
@benoit-cty
Copy link
Contributor Author

@benoit-cty that's a lot of added tests to review, and some checks fail. Could you please clarify if you wrote these tests yourself, or if they were generated by an LLM? If so, which review mechanism did you put in place to ensure that all these tests are relevant and warrant another human to review them? Thank you!

These tests was indeed added by Copilot to improve the test coverage on the area where @benjello discovered an error.

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.

Error not raised when building an erroneous reform
3 participants