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

fix(inline recs): Crash when no existing div.ars #763

Merged
merged 2 commits into from
May 2, 2024

Conversation

jesus2099
Copy link
Contributor

@jesus2099 jesus2099 commented May 1, 2024

I noticed that this userscript relies on mb_INLINE-STUFF to run properly.

Example release with no AR, no AcoustID and no ISRC (quick! it is being removed!)
https://musicbrainz.org/release/1f3a402f-7265-42ea-803a-f3cf8c508e7c

Or any other releases with credits at bottom and no mb_INLINE-STUFF running
https://musicbrainz.org/release/b4e8f7f4-e35c-42eb-a043-9605e4784aff

@jesus2099 jesus2099 changed the title fix: Crash when no existing div.ars fix (inline recs): Crash when no existing div.ars May 1, 2024
@jesus2099 jesus2099 changed the title fix (inline recs): Crash when no existing div.ars fix(inline recs): Crash when no existing div.ars May 1, 2024
@jesus2099
Copy link
Contributor Author

I don't understand why my PR fails a test.
Initially, my commit was called fix: Crash when no existing div.ars and it failed no tests.
Then I renamed it to fix(inline recs): Crash when no existing div.ars and now it fails the codecov/project test. 🤔

@jesus2099
Copy link
Contributor Author

jesus2099 commented May 1, 2024

Initially I forked repo to see if I could exclude current track from the inline track list.
I felt the current track* should not be displayed twice…
Then I found this problem. :D

Also I thought inline track list should be ordered, at least alphabetically (including the medium.track numbers).

* Current track, not current recording. As some releases have the same recording on several tracks.

@jesus2099
Copy link
Contributor Author

BTW I just noticed that that release had tracks with both span.mp and span.name-variation.
Those tracks are skipped as only one level of span is handled.
I added another commit.

@kellnerd
Copy link
Collaborator

kellnerd commented May 1, 2024

I don't understand why my PR fails a test.

No worries, it is the latest commit in main which is currently failing the code coverage test (because a few Discogs tests had to be disabled temporarily).

…pped

Tracks with pending edits and different title as recording have
both span.mp and span.name-variation.

    <td><span class="mp"><span class="name-variation"><a href="/recording/...

Only one level of span (either .mp or .name-variation) was handled.
@ROpdebee ROpdebee merged commit 811bb8b into ROpdebee:main May 2, 2024
10 of 11 checks passed
@ROpdebee
Copy link
Owner

ROpdebee commented May 2, 2024

Thanks for the fixes!

@jesus2099
Copy link
Contributor Author

@ROpdebee, you forgot the mb_qol_inline_recording_tracks label, that you can also add to #764.

@kellnerd kellnerd added mb_qol_inline_recording_tracks bug Something isn't working labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants