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

Make Kodi version getting safer #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Make Kodi version getting safer #12

wants to merge 2 commits into from

Conversation

glennguy
Copy link
Member

This way we still end up with the same data type to operate on -> avoid type errors

This way we still end up with the same data type to operate on -> avoid type errors
@glennguy
Copy link
Member Author

Hey @andybotting just wondering your thoughts on this :)

@andybotting
Copy link
Member

Have you had errors with this previously, or is it just to make it easier for testing?

Either way, I think it looks OK. Defensive coding is always a good thing

Copy link
Member

@andybotting andybotting left a comment

Choose a reason for hiding this comment

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

Couple of comments, but the code is good 👍

@@ -211,20 +211,26 @@ def get_kodi_build():
try:
return xbmc.getInfoLabel("System.BuildVersion")
except Exception:
return
return ''
Copy link
Member

Choose a reason for hiding this comment

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

Returning None is usually best if you're going to later test it with if build: for example. As you've done here, an empty string also evaluates to False. Does this necessarily need to be a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right - the changes in get_kodi_version make this redundant, I'll make it None.



def get_kodi_major_version():
"""Return the major version number of Kodi"""
version = get_kodi_version().split('.')[0]
return int(version)
if version:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like version should never evaluate to False from get_kodi_version(), so this change might not be that useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I worked from this function upwards, should have come back to look at it again!

@glennguy
Copy link
Member Author

Have you had errors with this previously, or is it just to make it easier for testing?

I've been checking the major version in a couple of cases recently, for either inputstream adaptive version checking or testing whether I can use more recent Kodi Python API calls.

What got me thinking about safeness is that I remember from a couple of years ago there was one of the xbmc.getInfoLabel calls that would sometimes return an empty string the first time it was called, and my feeling was that it is this one - System.BuildVersion. So just being a little defensive :)

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