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

Improve logging, file operations and Widevine CDM detection #310

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

mediaminister
Copy link
Collaborator

@mediaminister mediaminister commented Apr 12, 2020

This pull request takes improvements from #240 that are ready to be merged:

  • allow logging on different levels
  • wrap file operations in kodiutils.py
  • rework Widevine CDM detection functionality
  • various spelling and log messages corrections

@mediaminister mediaminister added this to the v0.4.6 milestone Apr 12, 2020
@michaelarnauts
Copy link
Contributor

Why do you use those magic constants with the log function? I very much prefer the real python logging interface where you can just use _LOGGER.debug() etc (like I did in viervijfzes), I'm clearly missing the point here :)

@mediaminister
Copy link
Collaborator Author

mediaminister commented Apr 12, 2020

To make sure add-on reviewers don't notice that we are logging to a forbidden log level. Silently breaking add-on rules to set a precedent.

@michaelarnauts
Copy link
Contributor

Nice and honest :)

@horstle
Copy link
Collaborator

horstle commented Apr 12, 2020

I think hardlink should rather be in utils instead of kodiutils. Other than that, looks good to me.

@mediaminister
Copy link
Collaborator Author

Okay, I moved the hardlink.

@mediaminister mediaminister merged commit 2d2fded into emilsvennesson:master Apr 14, 2020
@dagwieers
Copy link
Collaborator

Thank, I will rebase #240

@dagwieers
Copy link
Collaborator

BTW I am not so fond of moving hardlink out of kodiutils as we want kodiutils to become a body of work that can be reused by other projects, and the hardlink functionality, while not related directly to kodi is something we think may be useful for other add-on projects as well. It is not inputstreamhelper specific.

But that said, we will fix this when kodiutils has become a separate module that is reused by different projects.

@mediaminister
Copy link
Collaborator Author

Okay.

@horstle
Copy link
Collaborator

horstle commented Apr 14, 2020

Until now, kodiutils was a place to abstract kodi specific functions. hardlink is in no way related to kodi (neither directly nor indirectly). If I would search for a function creating hardlinks, with copy as fallback, I wouldn't look for it in a project called "kodiutils".

@michaelarnauts
Copy link
Contributor

I agree with @horstle. IMO kodiutils should only contain functions for kodi. Another example is caching. These functions are useful, but don't belong in kodiutils.

@dagwieers
Copy link
Collaborator

@michaelarnauts That is a strange thing to say when we discussed moving routing in the future kodiutils project. Same for the proxy-handling code.

The general idea was to have one body of work to assist writing add-ons,. If we go and fragment kodiutils now between every add-on using it, there's no point in sharing one codebase.

Caching may be something different as this may be harder to abstract. It may be very close to how a VOD provider stores it metadata.

@mediaminister
Copy link
Collaborator Author

mediaminister commented Apr 15, 2020

The general idea was to have one body of work to assist writing add-ons.

Potato, potahto. Can't we just name this future project addonutils and only group functions that are used by two or more add-ons?

@dagwieers
Copy link
Collaborator

@mediaminister That's the general idea. We postponed this for a few reasons:

  • Once shared it will be hard to impossible to redesign an interface (so we need versioned libs, and/or synchronised releases)
  • Some of the interfaces may benefit from a different interface (e.g. show_listing)
  • For the stubs there may be conflicting use-cases (and maybe the stubs ought to go elsewhere anyway)

@mediaminister mediaminister deleted the log branch December 28, 2020 14:00
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.

4 participants