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

[FIX] remove dangerous mutable default arguments in generate_artifacts #318

Merged
merged 1 commit into from
May 24, 2024

Conversation

clalancette
Copy link
Contributor

You cannot safely use = [] as a default argument in Python; subsequent calls will reuse that object. It probably doesn't matter in this case, as these are all "read-only" arguments, but it is better not to do it at all. Instead, we recognize that this function is always called with all of its arguments, so there is no need for default arguments at all.

While in here, I noticed that, generate_artifacts was returning a boolean on an error path, which is not allowed by its signature. So fix that as well.

You cannot safely use = [] as a default argument in
Python; subsequent calls will reuse that object.  It probably
doesn't matter in this case, as these are all "read-only"
arguments, but it is better not to do it at all.  Instead,
we recognize that this function is always called with
all of its arguments, so there is no need for default arguments
at all.

While in here, I noticed that, generate_artifacts was returning
a boolean on an error path, which is not allowed by its signature.  So fix
that as well.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.92%. Comparing base (fb22661) to head (b1739fd).

Files Patch % Lines
sros2/sros2/api/_artifact_generation.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           rolling     #318   +/-   ##
========================================
  Coverage    88.92%   88.92%           
========================================
  Files           24       24           
  Lines          614      614           
  Branches        64       64           
========================================
  Hits           546      546           
  Misses          50       50           
  Partials        18       18           
Flag Coverage Δ
unittests 88.92% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm!

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@mikaelarguedas mikaelarguedas changed the title Fix dangerous use of default arguments in python function call. [FIX] remove dangerous mutable default arguments in generate_artifacts May 24, 2024
@mikaelarguedas mikaelarguedas merged commit 2d8cb10 into rolling May 24, 2024
6 of 7 checks passed
@mikaelarguedas mikaelarguedas deleted the clalancette/fix-generate-artifacts branch May 24, 2024 06:38
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.

None yet

3 participants