search

payu-org / payu

A workflow management tool for numerical models on the NCI computing systems
Apache License 2.0
18 stars 26 forks source link

Empty YAML config handled as empty dict #252

Closed marshallward closed 3 years ago

marshallward commented 4 years ago

If one passes an empty YAML file, e.g. all lines commented out, then PyYAML returns None, which breaks subsequent assumptions in the handling of config.

We now parse an empty file as an empty dict, {}.

aidanheerdegen commented 4 years ago

Do we want to add a test to make sure this is picked up in case it gets accidentally removed/changed?

marshallward commented 4 years ago

Do you mean just adding a test case with an empty YAML file? Yeah, I can add that.

But you may mean something more significant, which I am missing.

aidanheerdegen commented 4 years ago

Yeah I meant an empty yaml test. I don't think it is super important, but it is a good habit to get into to add to the tests when a bug is fixed.

aidanheerdegen commented 3 years ago

I noticed this was still open, do you still feel like adding an empty yaml file test case @marshallward?

marshallward commented 3 years ago

Seems I cannot even remember how to run the tests (or perhaps they have changed). This may need a chat.

aidanheerdegen commented 3 years ago

@marshallward I added a test for an empty config, and found to my surprise it isn't empty because of that auto translation of the old collate boolean to a dict.

Are you happy with this behaviour, or do you want some special case logic around this

https://github.com/payu-org/payu/blob/master/payu/fsops.py#L72-L91

aidanheerdegen commented 3 years ago

Sorry, had to rebase and force push for tests to be consistent with current code base.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.04%) to 42.186% when pulling e2575c260702650304c400a53f1fd305ac66c69b on empty_config into f3f99e71198d104c0208fd74ad1690afdf21c2de on master.

marshallward commented 3 years ago

All good with me!

On Mon, Mar 29, 2021 at 1:21 AM Coveralls @.***> wrote:

[image: Coverage Status] https://coveralls.io/builds/38323652

Coverage increased (+0.04%) to 42.186% when pulling e2575c2 https://github.com/payu-org/payu/commit/e2575c260702650304c400a53f1fd305ac66c69b on empty_config into f3f99e7 https://github.com/payu-org/payu/commit/f3f99e71198d104c0208fd74ad1690afdf21c2de on master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/payu-org/payu/pull/252#issuecomment-809074019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQ32YEXTGZH2XJ4UZ7O5TTGAE5RANCNFSM4MXL2XJQ .