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

Added clearer error message when combiner script is run with incorrectly formatted yamls #55

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

uwagura
Copy link

@uwagura uwagura commented Jul 23, 2024

This PR addresses issue #54 . It adds a try-except clause to catch yamls that couldn't not be read because they contain a mixture of correctly formatted keys and incorrectly formatted keys, which address the problem that led to the ScannerError described in that issue. It also verifies that pyyaml doesn't read the yaml as a string, which address the "string indices must be integers" problem described in the issue.

Copy link
Contributor

@ceblanton ceblanton left a comment

Choose a reason for hiding this comment

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

Thanks @uwagura, this clearly improves the verbosity of reported problems.

I'm a bit confused about the two similar Exceptions, though. The first one prints the parsing error if the yaml cannot be loaded (i.e. safe_load).

The second one tests whether the parsed JSON is a string, and if so, raises an error as well. Shouldn't this always be a dictionary if safe_load worked?

@uwagura
Copy link
Author

uwagura commented Jul 23, 2024

Shouldn't this always be a dictionary if safe_load worked?

The issue is that safe_load doesn't return a dictionary if the yaml is incorrectly formatted. In particular, if all of the key-value pairs are incorrect - which was the case when I had title:$title and base_date:$baseDate in the diagYaml tags of my xml - safe_load() will read the yaml and return it as a string instead of a dictionary. When you try and index this string a couple of lines later as if it were a dictionary, you get the string indices must be integers error that I was getting, so this if statement is mean to catch scenarios like this.

If you only have a couple of incorrectly formatted keys and the rest are ok, safe_load() should fail, which is why I added that try-except block around the line that loads the yaml.

Copy link
Contributor

@uramirez8707 uramirez8707 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

@ceblanton
Copy link
Contributor

Shouldn't this always be a dictionary if safe_load worked?

The issue is that safe_load doesn't return a dictionary if the yaml is incorrectly formatted. In particular, if all of the key-value pairs are incorrect - which was the case when I had title:$title and base_date:$baseDate in the diagYaml tags of my xml - safe_load() will read the yaml and return it as a string instead of a dictionary. When you try and index this string a couple of lines later as if it were a dictionary, you get the string indices must be integers error that I was getting, so this if statement is mean to catch scenarios like this.

If you only have a couple of incorrectly formatted keys and the rest are ok, safe_load() should fail, which is why I added that try-except block around the line that loads the yaml.

Thanks for the explanation!

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.

3 participants