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

Fetch non-numeric track indices from MusicBrainz and Discogs #2363

Merged
merged 12 commits into from
Jan 11, 2017

Conversation

mikeacameron
Copy link
Contributor

This is a PR in reference to issue #1831.

@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2017

Great; thanks for getting the ball rolling here!

For the name of the field, maybe alt_track would suffice? Or, alternatively, track_alt to make the relationship with the current track field clear?

Also, to finish things up, would you mind:

  • writing a short changelog entry that explains what's going on with that field
  • cleaning up the whitespace changes
  • taking a look at the tests, which look like they need updating

@@ -265,6 +265,7 @@ def album_info(release):
)
ti.disctitle = disctitle
ti.media = format
ti.alt_track_no = track['number']
Copy link
Member

Choose a reason for hiding this comment

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

To complete my understanding, what does this number field usually hold? Is it typically a number, but sometimes something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding from looking at a handful of XML dumps is that position always holds an integer index of the track and the number holds whatever the user inputted when adding tracks. If you had an album on a double-sided CD, tape, vinyl, etc., then there's the potential of having the number being something like A1, or whatever the case may be.

So it seems to me that simply adding this data should have zero impact with sorting.

Copy link
Member

Choose a reason for hiding this comment

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

Cool; that helps! Thanks for elaborating.

@mikeacameron
Copy link
Contributor Author

I can do what you requested.

I also like the idea of naming it track_alt for consistency.

@mikeacameron
Copy link
Contributor Author

I believe I got everything done.

@sampsyo
Copy link
Member

sampsyo commented Jan 11, 2017

Looks great; thanks!

I think the problem is related to adding a fixed attribute for the field, which isn't really necessary—I'm going to flip this back to a flexible attribute and see how things go.

No need for a built-in field for a simple string-type optional field like
this.
@sampsyo
Copy link
Member

sampsyo commented Jan 11, 2017

Looks like that worked! Thanks again for all your work; I'm merging now.

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.

2 participants