Add IComparable and IEquatable to LyricsPhrase

[nlogozzo]

In Tagger we use LyricsInfo.SyncrhonizedLyrics.SequenceEqual which compares each item of the list with another SyncrhonizedLyrics list to see if the LyricsPhrase objects are the same.

The problem is SequenceEqual relies on the equality operator which was not implemented for LyricsPhrase. This PR fixes that. I'd like a release after this is merged please ❤️ (after #220 😅)

[Zeugma440]

Looks good 👍 Thanks for your contribution

[codecov[bot]]

Codecov Report

Patch coverage has no change and project coverage change: -0.11% :warning:

Comparison is base (b11c681) 89.25% compared to head (6684045) 89.14%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #219 +/- ## ========================================== - Coverage 89.25% 89.14% -0.11% ========================================== Files 93 93 Lines 18699 18722 +23 Branches 2750 2757 +7 ========================================== Hits 16689 16689 - Misses 1444 1467 +23 Partials 566 566 ``` | [Files Changed](https://app.codecov.io/gh/Zeugma440/atldotnet/pull/219?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [ATL/Entities/LyricsInfo.cs](https://app.codecov.io/gh/Zeugma440/atldotnet/pull/219?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-QVRML0VudGl0aWVzL0x5cmljc0luZm8uY3M=) | `79.64% <0.00%> (-20.36%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[sonarcloud[bot]]

SonarCloud Quality Gate failed.   

1 Bug
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. Read more here

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[Zeugma440]

Could you check that please?

https://sonarcloud.io/project/issues?resolved=false&severities=BLOCKER%2CCRITICAL%2CMAJOR%2CMINOR&sinceLeakPeriod=true&types=BUG&id=Zeugma440_atldotnet&open=AYob8Zl6mvm0rEb6AADM

[nlogozzo]

Could you check that please?

https://sonarcloud.io/project/issues?resolved=false&severities=BLOCKER%2CCRITICAL%2CMAJOR%2CMINOR&sinceLeakPeriod=true&types=BUG&id=Zeugma440_atldotnet&open=AYob8Zl6mvm0rEb6AADM

I think we can ignore that warning...If I implement GetHashCode() as such:

/// <summary>
/// Gets the hash code of the object
/// </summary>
/// <returns>The object's hash code</returns>
public override int GetHashCode() => TimestampMs ^ Text.GetHashCode();

I get a new warning of Non-readonly property references in GetHashCode() and since LyricsPhrase has no readonly properties, it wouldn't make sense to implement this...see https://stackoverflow.com/questions/20604039/non-readonly-fields-referenced-in-gethashcode for more detail

[Zeugma440]

IMHO, I'd rather have the 2nd warning than the 1st, given their respective consequences.

Could you add your implementation of GetHashCode() in a new PR, please?

[nlogozzo]

IMHO, I'd rather have the 2nd warning than the 1st, given their respective consequences.

I could, but I think implementing GetHashCode() has worse consequences...because if you were to add it to a Dictionary that uses hashing, the code will change and get messed up if we change the timestamp and/or text.

I see 2 ways to solve this:

1. Don't implement it and let it use object's default implementation which is just chose a random code but at least it won't change (the way it is now)

2. Make timestamp an init-only variable so we can use that for the hashcode and guarantee it won't change. Everytime we want to change the timestamp then we'd have to make a new object. This might make sense to change SynchronizedLyrics to a Dictionary of <int, LyricsPhrase>...

[Zeugma440]

I could, but I think implementing GetHashCode() has worse consequences...because if you were to add it to a Dictionary that uses hashing, the code will change and get messed up if we change the timestamp and/or text.

Well, that means neither of those options are good.

Don't implement it and let it use object's default implementation which is just chose a random code but at least it won't change (the way it is now)

I'm really uncomfortable having a custom Equals without its corresponding HashCode

Make timestamp an init-only variable so we can use that for the hashcode and guarantee it won't change. Everytime we want to change the timestamp then we'd have to make a new object

This looks like the only reasonable choice we have. We'd also have to do the same for Text. As an API consumer, are you okay with the consequences of it?

This might make sense to change SynchronizedLyrics to a Dictionary of <int, LyricsPhrase>

Semantically, I have a hard time seeing why we should adopt that. Timestamps are not an index one might want to query through a Dictionary 🤔

---

Also, that's kinda unrelated, but when redefining operators, why do you test a.Text.CompareTo(b.Text) against constants (-1 and 11)? Specs say they should return positive / zero / negative values.

[nlogozzo]

Also, that's kinda unrelated, but when redefining operators, why do you test a.Text.CompareTo(b.Text) against constants (-1 and 11)? Specs say they should return positive / zero / negative values.

Ah that's a bug. The -1 is right but 11 should be 1. With CompareTo, 1 means it's greater than, -1 means less than, 0 means equals to. I'll fix that in a PR

Semantically, I have a hard time seeing why we should adopt that. Timestamps are not an index one might want to query through a Dictionary 🤔

Yeah i guess you're right we can keep list.

This looks like the only reasonable choice we have. We'd also have to do the same for Text. As an API consumer, are you okay with the consequences of it?

Yes that's fine with me...i'll draft up a PR with everything now.

[nlogozzo]

See https://github.com/Zeugma440/atldotnet/pull/222

[Zeugma440]

That should be fine by now. I've done some extra cleanup thanks to resharper : 8fa50adf9d9e1f2509a59524bd9d423654302ed7

[nlogozzo]

That should be fine by now. I've done some extra cleanup thanks to resharper : 8fa50adf9d9e1f2509a59524bd9d423654302ed7

Should be good for release now