Skip to content
This repository was archived by the owner on Jun 25, 2020. It is now read-only.

Fix unittests for use with PyYAML 5.1 #137

Closed
rhaschke wants to merge 1 commit into from
Closed

Fix unittests for use with PyYAML 5.1 #137

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/wstool/config_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,11 +442,8 @@ def generate_config_yaml(config, filename, header, pretty=False,
items = [x.get_legacy_yaml(spec, exact) for x in items]

if items:
if pretty:
content += yaml.safe_dump(items, allow_unicode=True,
default_flow_style=False)
else:
content += yaml.safe_dump(items)
content += yaml.safe_dump(items, allow_unicode=True,
Copy link
Copy Markdown
Contributor

@rsinnet rsinnet Jun 20, 2019

Choose a reason for hiding this comment

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

This seems to change the behavior of the function. Before the change, allow_unicode was not explicitly set to True if pretty mode is disabled.

Copy link
Copy Markdown
Author

@rhaschke rhaschke Jun 20, 2019

Choose a reason for hiding this comment

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

That's true. But I couldn't see a reason to disallow unicode when pretty is False. Is there one?

Copy link
Copy Markdown
Contributor

@rsinnet rsinnet Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure in this case, but in general, I think it's good to not change something without understanding the implications or intent as this can lead to subtle bugs. One approach is to use git blame and track down the author to see if he or she remembers. That being said, I think this should be a separate PR if we do want to change it since it's a separate and perhaps unnecessary change.

Copy link
Copy Markdown
Author

@rhaschke rhaschke Jun 21, 2019

Choose a reason for hiding this comment

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

You are right. I'm going to close this PR as #133 is a better fix for this in this regard.
However, would be great if somebody could actually merge these critical fixes (#133 and #134).

default_flow_style=False if pretty else None)

if filename:
config_filepath = filename if os.path.isabs(filename) else \
Expand Down