Use ruamel.yaml for formatting and outputting rip .log file #415

itismadness · 2019-09-17T01:37:57Z

Similar to #384, but uses ruamel.yaml instead producing a YAML 1.2 compliant log, while retaining the ordering of they keys. There's still some steps of cleanup for values (handful of "%s" and "%d" replaces that can just use the value directly) and then matter of newlines in the log.

Here is a test script for checking the output of the logger (relies on having box), without having to actually rip a CD:

from __future__ import print_function
from box import Box
import ruamel.yaml
from whipper.result.logger import WhipperLogger


class Track(object):
    def __init__(self, number, start, end):
        self.number = number
        self.absolute = self.start = start
        self.end = end

    def getIndex(self, num):
        if num == 0:
            raise KeyError
        else:
            return self


class Table(object):
    def __init__(self):
        self.tracks = [
            Track(1, 0,  16263),
            Track(2, 16264, 33487)
        ]

    def getCDDBDiscId(self):
        return "c30bde0d"

    def getMusicBrainzDiscId(self):
        return "eyjySLXGdKigAjY3_C0nbBmNUHc-"

    def getMusicBrainzSubmitURL(self):
        return "https://musicbrainz.org/cdtoc/attach?toc=1+13+228039+150+16414+33638+51378+69369+88891+104871+121645+138672+160748+178096+194680+212628&tracks=13&id=eyjySLXGdKigAjY3_C0nbBmNUHc-"

    def getTrackLength(self, number):
        return self.tracks[number-1].end - self.tracks[number-1].start + 1

    def getTrackEnd(self, number):
        return self.tracks[number-1].end


ripResult = {
    "vendor": "HL-DT-STBD-RE  ",
    "model": "WH14NS40",
    "release": "1.03",
    "cdparanoiaVersion": "cdparanoia III 10.2 libcdio 2.0.0 x86_64-pc-linux-gnu",
    "cdparanoiaDefeatsCache": True,
    "offset": 6,
    "overread": True,
    "cdrdaoVersion": "1.2.4",
    "isCdr": False,
    "artist": "Various Artists",
    "title": "Shark Tale: Motion Picture Soundtrack",
    "table": Table(),
    "metadata": False,
    #     "url": ""
    # },
    "tracks": [
        {
            "number": 1,
            "filename": "./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/01. Sean Paul & Ziggy Marley - Three Little Birds.flac",
            "pregap": False,
            "peak": 29503,
            "pre_emphasis": False,
            "copyspeed": 7,
            "quality": 1,
            "testcrc": 0x0025D726,
            "copycrc": 0x0025D726,
            "AR": {
                "v1": {
                    "DBConfidence": 14,
                    "DBCRC": "95E6A189",
                    "CRC": "95E6A189"
                },
                "v2": {
                    "DBConfidence": 11,
                    "DBCRC": "113FA733",
                    "CRC": "113FA733"
                }
            },
            "testduration": 10,
            "copyduration": 10
        },
        {
            "number": 2,
            "filename": "./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/02. Christina Aguilera feat. Missy Elliott - Car Wash (Shark Tale mix).flac",
            "pregap": False,
            "peak": 31862,
            "pre_emphasis": False,
            "copyspeed": 7.7,
            "quality": 1,
            "testcrc": 0xF77C14CB,
            "copycrc": 0xF77C14CB,
            "AR": {
                "v1": {
                    "DBConfidence": 14,
                    "DBCRC": "0B3316DB",
                    "CRC": "0B3316DB"
                },
                "v2": {
                    "DBConfidence": 10,
                    "DBCRC": "A0AE0E57",
                    "CRC": "A0AE0E57"
                }
            },
            "testduration": 10,
            "copyduration": 10
        }
    ]
}

ripResult = Box(ripResult)


logger = WhipperLogger()
result = logger.log(ripResult)
print(result)
yaml = ruamel.yaml.YAML()
ruamel.yaml.dump(yaml.load(result))

itismadness · 2019-09-17T01:39:19Z

Old Logger Output:

Log created by: whipper 0.7.4.dev77+g635113b (internal logger)
Log creation date: 2019-09-17T01:38:36Z

Ripping phase information:
  Drive: HL-DT-STBD-RE  WH14NS40 (revision 1.03)
  Extraction engine: cdparanoia cdparanoia III 10.2 libcdio 2.0.0 x86_64-pc-linux-gnu
  Defeat audio cache: true
  Read offset correction: +6
  Overread into lead-out: true
  Gap detection: cdrdao 1.2.4
  CD-R detected: false

CD metadata:
  Release: Various Artists - Shark Tale: Motion Picture Soundtrack
  CDDB Disc ID: c30bde0d
  MusicBrainz Disc ID: eyjySLXGdKigAjY3_C0nbBmNUHc-
  MusicBrainz lookup URL: https://musicbrainz.org/cdtoc/attach?toc=1+13+228039+150+16414+33638+51378+69369+88891+104871+121645+138672+160748+178096+194680+212628&tracks=13&id=eyjySLXGdKigAjY3_C0nbBmNUHc-

TOC:
  1:
    Start: 00:00:00
    Length: 03:36:64
    Start sector: 0
    End sector: 16263

  2:
    Start: 03:36:64
    Length: 03:49:49
    Start sector: 16264
    End sector: 33487

Tracks:
  1:
    Filename: ./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/01. Sean Paul & Ziggy Marley - Three Little Birds.flac
    Peak level: 0.900360
    Pre-emphasis: false
    Extraction speed: 7.0 X
    Extraction quality: 100.00 %
    Test CRC: 0025D726
    Copy CRC: 0025D726
    AccurateRip v1:
      Result: Found, exact match
      Confidence: 14
      Local CRC: 95E6A189
      Remote CRC: 95E6A189
    AccurateRip v2:
      Result: Found, exact match
      Confidence: 11
      Local CRC: 113FA733
      Remote CRC: 113FA733
    Status: Copy OK

  2:
    Filename: ./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/02. Christina Aguilera feat. Missy Elliott - Car Wash (Shark Tale mix).flac
    Peak level: 0.972351
    Pre-emphasis: false
    Extraction speed: 7.7 X
    Extraction quality: 100.00 %
    Test CRC: F77C14CB
    Copy CRC: F77C14CB
    AccurateRip v1:
      Result: Found, exact match
      Confidence: 14
      Local CRC: 0B3316DB
      Remote CRC: 0B3316DB
    AccurateRip v2:
      Result: Found, exact match
      Confidence: 10
      Local CRC: A0AE0E57
      Remote CRC: A0AE0E57
    Status: Copy OK

Conclusive status report:
  AccurateRip summary: All tracks accurately ripped
  Health status: No errors occurred
  EOF: End of status report

SHA-256 hash: 6D6BCAFABAD3C55EF21C508ACB34D1A0C0D8BDCD02DE22B93D407B13E3BE8E52

New logger:

Log created by: whipper 0.7.4.dev78+g4013f51 (internal logger)
Log creation date: '2019-09-17T01:38:57Z'
Ripping phase information:
  Drive: HL-DT-STBD-RE  WH14NS40 (revision 1.03)
  Extraction engine: cdparanoia cdparanoia III 10.2 libcdio 2.0.0 x86_64-pc-linux-gnu
  Defeat audio cache: true
  Read offset correction: '+6'
  Overread into lead-out: true
  Gap detection: cdrdao 1.2.4
  CD-R detected: false
CD metadata:
  Release: 'Various Artists - Shark Tale: Motion Picture Soundtrack'
  CDDB Disc ID: c30bde0d
  MusicBrainz Disc ID: eyjySLXGdKigAjY3_C0nbBmNUHc-
  MusicBrainz lookup URL: https://musicbrainz.org/cdtoc/attach?toc=1+13+228039+150+16414+33638+51378+69369+88891+104871+121645+138672+160748+178096+194680+212628&tracks=13&id=eyjySLXGdKigAjY3_C0nbBmNUHc-
TOC:
  1:
    Start: 00:00:00
    Length: 03:36:64
    Start sector: '0'
    End sector: '16263'
  2:
    Start: 03:36:64
    Length: 03:49:49
    Start sector: '16264'
    End sector: '33487'
Tracks:
  1:
    Filename: ./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/01. Sean Paul & Ziggy Marley - Three Little Birds.flac
    Peak level: '0.900360'
    Pre-emphasis: false
    Extraction speed: 7.0 X
    Extraction quality: 100.00 %
    Test CRC: 0025D726
    Copy CRC: 0025D726
    AccurateRip v1:
      Result: Found, exact match
      Confidence: '14'
      Local CRC: 95E6A189
      Remote CRC: 95E6A189
    AccurateRip v2:
      Result: Found, exact match
      Confidence: '11'
      Local CRC: 113FA733
      Remote CRC: 113FA733
    Status: Copy OK
  2:
    Filename: ./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/02. Christina Aguilera feat. Missy Elliott - Car Wash (Shark Tale mix).flac
    Peak level: '0.972351'
    Pre-emphasis: false
    Extraction speed: 7.7 X
    Extraction quality: 100.00 %
    Test CRC: F77C14CB
    Copy CRC: F77C14CB
    AccurateRip v1:
      Result: Found, exact match
      Confidence: '14'
      Local CRC: 0B3316DB
      Remote CRC: 0B3316DB
    AccurateRip v2:
      Result: Found, exact match
      Confidence: '10'
      Local CRC: A0AE0E57
      Remote CRC: A0AE0E57
    Status: Copy OK
Conclusive status report:
  Health Status: No errors occurred
  EOF: End of status report
SHA-256 hash: 22E1CF3109B2CCDE9CF958798FBCECAFE1CB8AAF9C5E01F72427EED04CEC3114

itismadness · 2019-09-17T01:47:24Z

I think the simplest solution to get the newlines back introduced is to have regex that run on the following rules:

insert newline after Log creation date
insert a newline after a dictionary ends and returns to top-level (e.g. like when Conclusive status report: closes)
Insert newline after a track closes (e.g. like after Status: Copy OK)

This is probably simpler than trying to create a custom dumper for ruamel.yaml. @JoeLametta is regex fine by you for this?

itismadness · 2019-09-18T11:01:07Z

Alright, made some changes, here is the new output:

Log created by: whipper 0.7.4.dev80+gcf1b73d.d20190918 (internal logger)
Log creation date: 2019-09-18T12:53:59Z

Ripping phase information:
  Drive: HL-DT-STBD-RE  WH14NS40 (revision 1.03)
  Extraction engine: cdparanoia cdparanoia III 10.2 libcdio 2.0.0 x86_64-pc-linux-gnu
  Defeat audio cache: true
  Read offset correction: +6
  Overread into lead-out: true
  Gap detection: cdrdao 1.2.4
  CD-R detected: false

CD metadata:
  Release: 'Various Artists - Shark Tale: Motion Picture Soundtrack'
  CDDB Disc ID: c30bde0d
  MusicBrainz Disc ID: eyjySLXGdKigAjY3_C0nbBmNUHc-
  MusicBrainz lookup URL: https://musicbrainz.org/cdtoc/attach?toc=1+13+228039+150+16414+33638+51378+69369+88891+104871+121645+138672+160748+178096+194680+212628&tracks=13&id=eyjySLXGdKigAjY3_C0nbBmNUHc-

TOC:
  1:
    Start: 00:00:00
    Length: 03:36:64
    Start sector: 0
    End sector: 16263

  2:
    Start: 03:36:64
    Length: 03:49:49
    Start sector: 16264
    End sector: 33487

Tracks:
  1:
    Filename: ./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/01. Sean Paul & Ziggy Marley - Three Little Birds.flac
    Peak level: 0.90036
    Pre-emphasis: false
    Extraction speed: 7.0 X
    Extraction quality: 100.00 %
    Test CRC: 0025D726
    Copy CRC: 0025D726
    AccurateRip v1:
      Result: Found, exact match
      Confidence: 14
      Local CRC: 95E6A189
      Remote CRC: 95E6A189
    AccurateRip v2:
      Result: Found, exact match
      Confidence: 11
      Local CRC: 113FA733
      Remote CRC: 113FA733
    Status: Copy OK

  2:
    Filename: ./soundtrack/Various Artists - Shark Tale - Motion Picture Soundtrack/02. Christina Aguilera feat. Missy Elliott - Car Wash (Shark Tale mix).flac
    Peak level: 0.972351
    Pre-emphasis: false
    Extraction speed: 7.7 X
    Extraction quality: 100.00 %
    Test CRC: F77C14CB
    Copy CRC: F77C14CB
    AccurateRip v1:
      Result: Found, exact match
      Confidence: 14
      Local CRC: 0B3316DB
      Remote CRC: 0B3316DB
    AccurateRip v2:
      Result: Found, exact match
      Confidence: 10
      Local CRC: A0AE0E57
      Remote CRC: A0AE0E57
    Status: Copy OK

Conclusive status report:
  Health Status: No errors occurred
  EOF: End of status report

SHA-256 hash: B1570B36EEDEA7B90DE81082B9CE68AA8784751BE532AA2B8CCD5877E4F75114

which generates mostly the same output as before through a series of regex replaces, except that all strings that might contain certain characters (like Release) will be properly quoted if need be (like in the above case).

@JoeLametta this should be ready to merge.

JoeLametta · 2019-10-01T09:44:33Z

First of all thanks for the pull request! Overall it seems ready to merge to me.

Just a few questions:

Isn't it possible to avoid single quoting the value of Log creation date
In your example of a log generated with the new logger I see that the AccurateRip summary line is missing (but according to the source code it should be there): is it just a mistake?
The old logger always represented the peak level with 6 decimal places, the new one does not (I don't think this is problem, just an observation):

Peak level: 0.900360
Peak level: 0.90036

Maybe it's unrelated to this pull request but I'm considering restructuring the logger output a bit so that breaking changes are minimized in the future.

Current idea: put Log created by and Log creation date inside a section (don't know which key would be appropriate as name) and separate the information about the logger used in a new line (in the same section).
The result would be something like this (everything else would remain the same):

Whipper section (name to be defined):
  Log created by: whipper 0.7.4.dev77+g635113b
  Logger used: internal logger
  Log creation date: 2019-09-17T01:38:36Z

Advantages:

The RegEX needed to insert a newline after the Log creation date line can be dropped
Information about the kind of logger used can be retrieved more easily
Clearer organization of information (maybe?)

Another idea is to add a field to the logger to version it (signaling breaking changes).

@itismadness What do you think about these ideas? Any other suggestions?

itismadness · 2019-10-06T13:53:52Z

Isn't it possible to avoid single quoting the value of Log creation date

Yes, but ruamel.yaml unfortunately insists on putting quotes around it with the fix there being one of:

regex to remove the quotes (what I'm doing)
dig into ruamel.yaml and create a custom dumper to not add the strings (way too much work for this)

In your example of a log generated with the new logger I see that the AccurateRip summary line is missing (but according to the source code it should be there): is it just a mistake?

Yes, fixed in d01bf08f40c4b5ebf445b967375d4429a6df7f7f.

The old logger always represented the peak level with 6 decimal places, the new one does not (I don't think this is problem, just an observation):

Yup, missed removing one of the pointless %d formats (which you found and fixed, thanks)

@itismadness What do you think about these ideas? Any other suggestions?

Similar to my suggestion in #416, I'm on board with splitting multi-value fields apart to make it easier for automated tools to consume. The whipper version line is well-structured enough to make it easy to split apart, but in an ideal world (for me), it'd be cool to see:

Log Details:
  Log created by:
    Ripper: whipper
    Version: 0.7.4.dev77+g635113b
  Logger used: internal logger
  Log creation date: 2019-09-17T01:38:36Z

I'm not sure it's really necessary to version the internal logger itself as that's wrapped up in the version of whipper anyway and I'm already doing necessary compatibility shifts based on that. The only reason I would see to add that is if there were many different external loggers people were plugging into whipper, but I am not sure how prevalent that actually is (and I personally would rather see everyone use this nice yaml based structured instead of something like EAC structure which requires way too much regex to parse).

JoeLametta · 2019-10-20T14:16:54Z

Everything looks good! Could you also commit a test case for the logger?

JoeLametta · 2019-10-22T09:43:45Z

I've rebased on develop, please check if this makes sense (now that artist and title are separated - 4569644):

https://github.com/whipper-team/whipper/blob/23e75936ea4544b975dd56d9ad00b6bf946f856f/whipper/result/logger.py#L57-L69

itismadness · 2019-10-22T21:30:50Z

That looks fine. I'll add a testcase (swapping out the use of Box in my homegrown test for just manually constructing some dummy test objects) in the next couple of days (probably this weekend).

JoeLametta · 2019-10-26T10:16:50Z

Hi, I think this is the last sizable change left to merge before tagging a new whipper's release (v0.8.0). 🎆

itismadness · 2019-10-26T15:10:52Z

Alright @JoeLametta, updated with a unittest.TestCase to verify the new logger code works. I used the actual classes for RipResult and TrackResult, but mocked whipper.image.table as that looks to be a bit more complex in giving fake data.

The testcase then:

verifies that a log is written out to a string
verifies that the body (minus header and hash) match a stored file (as those three lines will change)
verifies the header via regex
verifies that the written out log is indeed valid yaml (by round-tripping it through ruamel)
as part of round-trip, get the sha-256 hash, and then verify that it matches the generated log (testing method I use downstream to verify)

I also modified the Travis setup so that it uses requirements.txt for everything but pycdio (which gets installed separately) which makes it a bit easier to add new dependencies in the future as only have to edit one place, not two.

- insert newlines after blocks of content - strip quotes around creation time and offset - Change offset value representation - Clarify logger regular expressions - Fix missing AccurateRip Summary field - Read usage of time.gmtime(epoch) for generating datetime string - fix missing dependency in travis - install pycdio with specific version separately Signed-off-by: itismadness <itismadness@users.noreply.github.com>

Signed-off-by: itismadness <itismadness@users.noreply.github.com>

JoeLametta · 2019-10-27T08:11:58Z

Merged, thanks!

accuraterip-checksum converted to C extension: whipper-team/whipper#274 Switch to YAML library (ruamel) for formatting log files: whipper-team/whipper#415 Port to Python 3: whipper-team/whipper#411

itismadness changed the title ~~WIP: Use ruamel.yaml for formatting rip .log file~~ Use ruamel.yaml for formatting and outputting rip .log file Sep 18, 2019

itismadness marked this pull request as ready for review September 18, 2019 10:33

JoeLametta mentioned this pull request Oct 21, 2019

Use PyYAML for formatting the rip .log file #384

Closed

JoeLametta force-pushed the ruamel_yaml branch from b71ec9f to 23e7593 Compare October 22, 2019 09:38

JoeLametta added this to the 0.8.0 milestone Oct 23, 2019

itismadness added 2 commits October 27, 2019 08:00

write unit test case for WhipperLogger

3cd2da7

Signed-off-by: itismadness <itismadness@users.noreply.github.com>

JoeLametta force-pushed the ruamel_yaml branch from f217c8a to 3cd2da7 Compare October 27, 2019 08:08

JoeLametta merged commit 8e60fb9 into whipper-team:develop Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ruamel.yaml for formatting and outputting rip .log file #415

Use ruamel.yaml for formatting and outputting rip .log file #415

itismadness commented Sep 17, 2019 •

edited

Loading

itismadness commented Sep 17, 2019

itismadness commented Sep 17, 2019

itismadness commented Sep 18, 2019

JoeLametta commented Oct 1, 2019

itismadness commented Oct 6, 2019 •

edited

Loading

JoeLametta commented Oct 20, 2019

JoeLametta commented Oct 22, 2019 •

edited

Loading

itismadness commented Oct 22, 2019

JoeLametta commented Oct 26, 2019

itismadness commented Oct 26, 2019 •

edited

Loading

JoeLametta commented Oct 27, 2019

Use ruamel.yaml for formatting and outputting rip .log file #415

Use ruamel.yaml for formatting and outputting rip .log file #415

Conversation

itismadness commented Sep 17, 2019 • edited Loading

itismadness commented Sep 17, 2019

itismadness commented Sep 17, 2019

itismadness commented Sep 18, 2019

JoeLametta commented Oct 1, 2019

itismadness commented Oct 6, 2019 • edited Loading

JoeLametta commented Oct 20, 2019

JoeLametta commented Oct 22, 2019 • edited Loading

itismadness commented Oct 22, 2019

JoeLametta commented Oct 26, 2019

itismadness commented Oct 26, 2019 • edited Loading

JoeLametta commented Oct 27, 2019

itismadness commented Sep 17, 2019 •

edited

Loading

itismadness commented Oct 6, 2019 •

edited

Loading

JoeLametta commented Oct 22, 2019 •

edited

Loading

itismadness commented Oct 26, 2019 •

edited

Loading