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

Error when parsing log file due to left pad track number #340

Closed
itismadness opened this issue Nov 29, 2018 · 3 comments
Closed

Error when parsing log file due to left pad track number #340

itismadness opened this issue Nov 29, 2018 · 3 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Priority: low Low priority Sprintable Small enough to sprint on
Milestone

Comments

@itismadness
Copy link
Contributor

itismadness commented Nov 29, 2018

Right now, whipper left pads the track numbers out for the TOC and Tracks top-level entries in the YAML log such that each entry is of equal length. When parsing it, the YAML Spec specifies that the values 01 to 07 be treated as octals and thus how 08 and 09 parses is somewhat undefined as ends up implementation dependent.

Implementations tested:

  • php_yaml will parse them all to integers
  • symfony/yaml will parse 01-07 to integers and then 08 and 09 to integer 0 (raising an exception when they're both there)
  • PyYAML will parse 01-07 as integers, and then 08 and 09 to strings.

Minimum example to test parsing:

01: Yes
02: No
03: Yes
04: No
05: Yes
06: No
07: Yes
08: No
09: Yes
10: No
@JoeLametta
Copy link
Collaborator

Thanks for reporting this issue.
I think there are two ways to solve this: either dropping leading zeros from the track number or quoting track numbers in the logfile. What do you suggest?

@JoeLametta JoeLametta added Bug Generic bug: can be used together with more specific labels Accepted Accepted issue on our roadmap labels Nov 29, 2018
@JoeLametta JoeLametta added this to the 1.0 milestone Nov 29, 2018
@JoeLametta JoeLametta added Priority: low Low priority Sprintable Small enough to sprint on Needed: patch A pull request is required Needed: discussion More discussion needed before anything can be done (or still no agreement has been reached) labels Nov 29, 2018
@itismadness
Copy link
Contributor Author

I'd suggest dropping the leading zero as I think that'd look more natural to end users looking at the outputted log?

JoeLametta added a commit that referenced this issue Nov 29, 2018
@JoeLametta
Copy link
Collaborator

I'd suggest dropping the leading zero as I think that'd look more natural to end users looking at the outputted log?

I agree. I've just fixed this issue in 4a53ecb.

@JoeLametta JoeLametta self-assigned this Nov 29, 2018
jtl999 pushed a commit to jtl999/whipper that referenced this issue Dec 8, 2018
jtl999 pushed a commit to jtl999/whipper that referenced this issue Dec 8, 2018
jtl999 pushed a commit to jtl999/whipper that referenced this issue Dec 9, 2018
jtl999 pushed a commit to jtl999/whipper that referenced this issue Dec 9, 2018
@JoeLametta JoeLametta removed Needed: discussion More discussion needed before anything can be done (or still no agreement has been reached) Needed: patch A pull request is required labels Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug Generic bug: can be used together with more specific labels Priority: low Low priority Sprintable Small enough to sprint on
Projects
None yet
Development

No branches or pull requests

2 participants