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

Use YAML 1.2 boolean values for generated log files #404

Closed
wants to merge 1 commit into from
Closed

Use YAML 1.2 boolean values for generated log files #404

wants to merge 1 commit into from

Conversation

itismadness
Copy link
Contributor

The current log files are generated using values that are considered booleans under YAML 1.1, but not YAML 1.2.

This makes the generated logs use the standard boolean values of true/false instead of Yes/No so that the logs are parsed the same under both YAML 1.1 and 1.2 into booleans which is going to be less surprising to downstream automated consumers of these logs.

The main point of contention I would see is that "Defeat audio cache" was changed from Unknown/Yes/No to null/true/false. All of the others were strict booleans (could only be true or false).

Example:

import yaml# PyYAML, uses YAML 1.1
from ruamel.yaml import YAML # uses YAML 1.2

ruamel_yaml = YAML(saf=True)

with open('whipper_test.log') as open_file:
    log = open_file.read()

print(yaml.safe_load(log)['Ripping phase information']['Defeat audio cache']) # True
print(ruamel_yaml.load(log)['Ripping phase information']['Defeat audio cache']) # Yes

This follows from the discussion in #384 around this.

@itismadness itismadness changed the title Use YAML 1.2 boolean values for booleans Use YAML 1.2 boolean values for boolean key/value pairs Jun 10, 2019
@itismadness itismadness changed the title Use YAML 1.2 boolean values for boolean key/value pairs Use YAML 1.2 boolean values for generated log files Jun 10, 2019
@MerlijnWajer
Copy link
Collaborator

We should make sure that this doesn't break existing logparsers.

@itismadness
Copy link
Contributor Author

This will 100% break any logparsers that use a YAML 1.2 parser as it'll go from expecting (and operating on) strings for these fields to them now being booleans. However, how many logparsers exist out in the wild and given the pre-release status of whipper, is it not acceptable to have these kinds of quality-of-life breaks?

@Freso
Copy link
Member

Freso commented Jun 11, 2019

We should make sure that this doesn't break existing logparsers.

In some previous discussion (can’t remember if it was on GitHub or in the #whipper IRC channel), either you or @JoeLametta said that it’s fine to break log parsers until we hit the 1.0.0 (and I agree with that).

We should make sure to at least bump the minor version though for the next 0.x.x release if we do this, but we should already be bumping the minor for next release anyway (due to some other small log change, I think?).

Also, I think we should absolutely be using YAML 1.2 if possible.

@JoeLametta
Copy link
Collaborator

@itismadness Thanks for the pull request: LGTM. In order to merge it without CI failures, please add a Signed-off-by line in the commit's description as documented here.

@JoeLametta
Copy link
Collaborator

Merged in 0d69258.

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.

4 participants