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 Connector API handling of undefined/null values #3689

Merged
merged 3 commits into from
May 3, 2023
Merged

Fix Connector API handling of undefined/null values #3689

merged 3 commits into from
May 3, 2023

Conversation

yayuyokitano
Copy link
Member

Some functions in connector do not handle undefined/null values correctly.
This goes a long way to fixing that, and adjusts connectors accordingly.

I think once these three current PRs are merged, I want to do one last PR changing the sidebar organization in the options (I think there will be a lot of merge conflicts if this is done before current PRs are merged and I can't be bothered) and then I think we might be ready to release!

@yayuyokitano yayuyokitano added core This issue or pull request is related to the extension core fixed labels May 2, 2023
src/core/content/util.ts Outdated Show resolved Hide resolved
@yayuyokitano yayuyokitano requested a review from inverse May 3, 2023 10:55
@@ -307,7 +307,9 @@ export default class BaseConnector {
* @param trackArtUrl - Track art URL
* @returns Check result
*/
public isTrackArtDefault: (trackArtUrl?: string) => boolean = () => false;
public isTrackArtDefault: (
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to do this? Feels like this function should always return bool

Copy link
Member

@inverse inverse left a comment

Choose a reason for hiding this comment

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

LGTM although the interface feels weird but your points are totally valid :)

@yayuyokitano
Copy link
Member Author

Yeah it's a little odd – why i didn't do this in the initial v3 implementation.

btw I wonder if we're good to release now. I can't think of anything more at this point (though im sure there are some bugs hiding)

@yayuyokitano yayuyokitano merged commit 6032f69 into web-scrobbler:master May 3, 2023
8 checks passed
@inverse
Copy link
Member

inverse commented May 3, 2023

Feels like we're in a very good shape with a lot of nice additions from you ;)

Pushed the latest translations - should probably look into syncing those automatically but we're in a good place right now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core This issue or pull request is related to the extension core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants