Skip to content

Commit

Permalink
Include MusicBrainz Release URL in log output (#382)
Browse files Browse the repository at this point in the history 
* Include MusicBrainz Release URL in log output

This also passes *all* metadata to the `result` object, giving loggers a
lot more (release) metadata to work with, in case custom, “3rd party”
loggers (or even ourselves in the future!) want to do something more
fancy or expansive with the metadata in the log file.

Fixes #381

Signed-off-by: Frederik “Freso” S. Olesen <freso.dk@gmail.com>

* Uppercase "url" in output: "URL"

Signed-off-by: Frederik “Freso” S. Olesen <freso.dk@gmail.com>
  • Loading branch information
@Freso @JoeLametta
Freso authored and JoeLametta committed Mar 18, 2019
1 parent a9bb51a commit 135cc9c
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 1 deletion.
1 change: 1 addition & 0 deletions whipper/command/cd.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ def do(self):
_, self.program.result.vendor, self.program.result.model, \
self.program.result.release = \
cdio.Device(self.device).get_hwinfo()
self.program.result.metadata = self.program.metadata

self.doCommand()

Expand Down
5 changes: 4 additions & 1 deletion whipper/result/logger.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,11 @@ def logRip(self, ripResult, epoch):
lines.append(" CDDB Disc ID: %s" % ripResult. table.getCDDBDiscId())
lines.append(" MusicBrainz Disc ID: %s" %
ripResult. table.getMusicBrainzDiscId())
lines.append(" MusicBrainz lookup url: %s" %
lines.append(" MusicBrainz lookup URL: %s" %
ripResult. table.getMusicBrainzSubmitURL())
if ripResult.metadata:
lines.append(" MusicBrainz Release URL: %s" %
ripResult.metadata.url)
lines.append("")

# TOC section
Expand Down
3 changes: 3 additions & 0 deletions whipper/result/result.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class RipResult:
@ivar offset: sample read offset
@ivar table: the full index table
@type table: L{whipper.image.table.Table}
@ivar metadata: disc metadata from MusicBrainz (if available)
@type metadata: L{whipper.common.mbngs.DiscMetadata}
@ivar vendor: vendor of the CD drive
@ivar model: model of the CD drive
Expand All @@ -88,6 +90,7 @@ class RipResult:
table = None
artist = None
title = None
metadata = None

vendor = None
model = None
Expand Down

3 comments on commit 135cc9c

@Freso
Copy link
Member Author

@Freso Freso commented on 135cc9c Mar 19, 2019

Choose a reason for hiding this comment

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

I really, really dislike the squashing of commits with distinct changes. If you had preferred, I could have made one change changing "MusicBrainz lookup url" to "MusicBrainz lookup URL" and then followed up with a change to add the MB Release URL to the log in another commit, but they are functionally two distinct and unrelated changes and should not have been mashed up into the same commit. :(

On top of that (I'm undecided on whether it's less or more severe than non-atomic commits), squashing the commits also means that my GPG signing of the commits is gone. The "sign-off" is much less valid now, as there's nothing to verify that it was actually me making the commits in the first place and thus some level of verifiability is lost with this squash merge commit.

Can we please reconsider how we merge things?

@JoeLametta
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really, really dislike the squashing of commits with distinct changes. If you had preferred, I could have made one change changing "MusicBrainz lookup url" to "MusicBrainz lookup URL" and then followed up with a change to add the MB Release URL to the log in another commit, but they are functionally two distinct and unrelated changes and should not have been mashed up into the same commit. :(

I'm sorry. To me it seems not a big deal (considering the size and the impact of that change).

On top of that (I'm undecided on whether it's less or more severe than non-atomic commits), squashing the commits also means that my GPG signing of the commits is gone. The "sign-off" is much less valid now, as there's nothing to verify that it was actually me making the commits in the first place and thus some level of verifiability is lost with this squash merge commit.

Right, I think this is more severe.

Can we please reconsider how we merge things?

Sure, issue please? (this may become a documented policy)

@Freso
Copy link
Member Author

@Freso Freso commented on 135cc9c Mar 21, 2019

Choose a reason for hiding this comment

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

To me it seems not a big deal (considering the size and the impact of that change).

In this specific case, it's not, but when you start making value judgements like this, you'll end up making the wrong judgement at some point, which you may not realise until you're git bisecting 1½ year later down the road. :/

issue please

I'll try and whip one up!

Please sign in to comment.