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
vcstools:masterfrom
rhaschke:fix-unittests
Closed

Fix unittests for use with PyYAML 5.1#137
rhaschke wants to merge 1 commit into
vcstools:masterfrom
rhaschke:fix-unittests

Conversation

@rhaschke
Copy link
Copy Markdown

@rhaschke rhaschke commented Jun 20, 2019

According to the ChangeLog (https://pyyaml.org/wiki/PyYAML), PyYAML 5.1 changed
the default for the default_flow_style argument of dumpers, which makes all related unittests fail.
This addresses #136.

According to the ChangeLog (https://pyyaml.org/wiki/PyYAML), PyYAML 5.1 changed
the default for the `default_flow_style` argument of dumpers, which makes all related unittests fail.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 81.974% when pulling 85eac1b on rhaschke:fix-unittests into 3a6a2d3 on vcstools:master.

Comment thread src/wstool/config_yaml.py
default_flow_style=False)
else:
content += yaml.safe_dump(items)
content += yaml.safe_dump(items, allow_unicode=True,
Copy link
Copy Markdown
Contributor

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

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

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

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).

@rhaschke
Copy link
Copy Markdown
Author

Just noticed that this is a duplicate of #133. Please, @tfoote or @wjwwood merge one of these fixes.

@rhaschke
Copy link
Copy Markdown
Author

Closing in favour of #133.

@rhaschke rhaschke closed this Jun 21, 2019
@rhaschke rhaschke deleted the fix-unittests branch June 21, 2019 05:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants