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

Empty or incorrect config causes crashes instead of error messages #270

Closed
frol opened this Issue Aug 27, 2015 · 3 comments

Comments

Projects
None yet
3 participants
@frol

frol commented Aug 27, 2015

There are all sorts of possible errors with configs, but none are handled at this point.

Legal cases that cause crashes when they shouldn't:

  1. Empty invoke.yaml breaks on merge_dict because update data is None.
  2. Empty invoke.json raises ValueError in json module itself.

NOTE: When invoke.yaml contains one word (without colon), yaml parser returns a str object, which is also not a dict.

NOTE: Error handling is not implemented in Invoke yet, see #269. So it should get addressed first.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 2, 2015

Yup, no real smoke testing has been done on that feature yet. Thanks!

@presidento

This comment has been minimized.

Contributor

presidento commented Jan 2, 2016

I also run into this problem. While not touching other error handling related things, could we recognize empty config files as (valid) empty configs? (May I create a patch for it?)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 10, 2018

Have a proximate fix for this in now, prompted by #551. A little worried about the root cause though - why is a config level None instead of an empty dict?

#551 is phrased as this being caused by a runtime file (which would imply the issue is with that, relatively new, feature) but the traceback therein seems to show the problem being merging of a user-level file instead, which would mean it's a more generic problem.


At a glance it seems to be specifically that PyYAML turns an empty file into None instead of a dict; whether this is on-spec or not doesn't really matter, it's a thing that can happen.

I quickly looked to see what happens with other formats; unfortunately the json module straight up throws a JSONDecodeError if the file is blank instead of eg {}, and there's no nice way to mute that without muting actual "you had JSON and it was bad" or "you accidentally put YAML into a .json" or etc etc, errors.

So, sans a nice clean way to tackle this in a blanket fashion, I am just going to leave my fix as "merge_dicts gracefully handles None input".

bitprophet added a commit that referenced this issue Jul 10, 2018

bitprophet added a commit that referenced this issue Jul 10, 2018

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