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

Do not keep empty Node actions in serialized output #59

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

akx
Copy link
Member

@akx akx commented Nov 3, 2021

There's no need to have the optional empty list there if the input maybe wasn't there in the first place.

This is currently breaking a test in valohai/valohai-utils#75 since valohai-yaml got bumped to 0.20.0 there.

@akx akx added the bug label Nov 3, 2021
@akx akx requested review from a team, ruksi and magdapoppins and removed request for a team November 3, 2021 14:20
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2021

Codecov Report

Merging #59 (641bc3b) into master (7743b5c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   93.50%   93.54%   +0.03%     
==========================================
  Files          47       47              
  Lines        1325     1333       +8     
  Branches      177      178       +1     
==========================================
+ Hits         1239     1247       +8     
  Misses         44       44              
  Partials       42       42              
Impacted Files Coverage Δ
tests/test_pipeline.py 90.90% <100.00%> (+2.02%) ⬆️
valohai_yaml/objs/pipelines/node.py 96.87% <100.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7743b5c...641bc3b. Read the comment docs.

@akx akx force-pushed the no-serialize-empty-actions branch from 5129ba8 to 6578713 Compare November 3, 2021 17:20
There's no need to have the optional empty list there if the input maybe wasn't there in the first place.
@akx akx force-pushed the no-serialize-empty-actions branch from 6578713 to 641bc3b Compare November 4, 2021 07:22
Copy link
Member

@ruksi ruksi left a comment

Choose a reason for hiding this comment

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

lgtm!

Comment on lines +46 to +47
if not ser.get('actions'):
ser.pop('actions', None)
Copy link
Member

Choose a reason for hiding this comment

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

if actions falsy or non-existent, try to remove actions from the dict

the code could signify intent a bit more but that is mainly a personal preference 🤔

such a tiny snippet anyway + a test to back it up

@ruksi ruksi merged commit 1e9c75a into master Nov 4, 2021
@ruksi ruksi deleted the no-serialize-empty-actions branch November 4, 2021 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants