Skip to content

Conversation

@ioangatop
Copy link
Contributor

@ioangatop ioangatop commented Feb 15, 2023

What does this PR do?

Closes #241

Before submitting

  • Did you read the contributing guideline?
  • Did you update the documentation? (readme and public docstrings)
  • Did you write unit tests such that there is 100% coverage on related code? (required for bug fixes and new features)
  • Did you verify that new and existing tests pass locally?
  • Did you make sure that all changes preserve backward compatibility?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

Copy link
Member

@mauvilsa mauvilsa left a comment

Choose a reason for hiding this comment

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

@ioangatop thank you for the effort to contribute!

From what I see this pull request adds the fail_untyped parameter to add_dataclass_arguments. Indeed this is missing and it seems to resolve #241.

However, I see there is no unit test added. For every bug fix and new feature at least one unit test should be added, even if the coverage does not decrease with the code changes. Please convert the code snippet in the description #241 into a unit test.

@ioangatop
Copy link
Contributor Author

@ioangatop thank you for the effort to contribute!

From what I see this pull request adds the fail_untyped parameter to add_dataclass_arguments. Indeed this is missing and it seems to resolve #241.

However, I see there is no unit test added. For every bug fix and new feature at least one unit test should be added, even if the coverage does not decrease with the code changes. Please convert the code snippet in the description #241 into a unit test.

@mauvilsa added!

@mauvilsa mauvilsa added the enhancement New feature or request label Feb 16, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mauvilsa mauvilsa merged commit 73a2522 into omni-us:master Feb 16, 2023
@ioangatop
Copy link
Contributor Author

@mauvilsa thank you very much for the review and merge!

Question: because this feature is kinda important for us, do you know when you will tag a new version and release it?

Thank you again

@ioangatop ioangatop deleted the 241-fix-fail-untyped-for-dataclasses branch February 16, 2023 14:11
@mauvilsa
Copy link
Member

To release I need to run further tests with projects where we use jsonargparse. I am currently traveling and unable to do that. I will try to look into it early next week.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update fail_untyped flag to work with dataclasses whose arguments are classese with untyped arguments.

2 participants