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

Track: Fix debug assertion for missing/invalid bpm #4100

Merged
merged 1 commit into from
Jul 23, 2021
Merged

Track: Fix debug assertion for missing/invalid bpm #4100

merged 1 commit into from
Jul 23, 2021

Conversation

uklotzde
Copy link
Contributor

Occurred after applying changes in DlgTrackInfo.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1023011203

  • 3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 28.629%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/track/track.cpp 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
src/engine/cachingreader/cachingreader.cpp 9 71.38%
Totals Coverage Status
Change from base Build 1022999157: -0.01%
Covered Lines: 20089
Relevant Lines: 70169

💛 - Coveralls

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

Any way to add a test for this? otherwise lgtm

@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 12, 2021

You cannot test a debug assertion.

The invocation of Bpm::value() was wrong. Now it is gone. Nothing to test.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. As discussed on Zulip I won't hit merge until @ywwg had the chance to re-review (as he used "Request Changes" instead of "Comment").

@Holzhaus Holzhaus requested a review from ywwg July 13, 2021 07:36
@uklotzde
Copy link
Contributor Author

@ywwg Ping. We already discussed the liabilities that come with Request changes.

@Holzhaus
Copy link
Member

@ywwg Ping.

@Holzhaus
Copy link
Member

@ywwg ping again. It would nice to get this fix into main soon.

@uklotzde
Copy link
Contributor Author

There is no point in discussing review practices and contribution guidelines if even the basic rules of communication are not obeyed by ourselves. As a first-time contributor I would have closed the PR left now.

@ywwg
Copy link
Member

ywwg commented Jul 23, 2021

@ywwg Ping.

I have no idea why github didn't tell me about these pings :(. I am constantly trying to fix my notifications so I only get told about things that are important, but I am buried with notifications about every single PR that happens. Sorry for the delay.

@Swiftb0y
Copy link
Member

image
fyi you can restrict for what activities you get notifications for and you can also filter by the reason you got notified for.
image
Also please double check some of the recent PRs, there are a bunch that require your attention and you seem to have missed them so far.

@Holzhaus Holzhaus merged commit c797a81 into mixxxdj:main Jul 23, 2021
@uklotzde uklotzde deleted the track-set-bpm branch July 23, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants