File-like objects support #45

JuniorJPDJ · 2021-05-12T01:35:44Z

and simple and useful MediaFile.as_dict() function

Resolves #35

sampsyo · 2021-05-12T14:56:58Z

Thanks for getting this started! If you're interested in changing path to filename, perhaps we should do that in a separate PR so it's easier to review the core of the "file-like object" feature separately? (In general, for me, I find it's easier to fully understand one change at a time.)

JuniorJPDJ · 2021-05-12T15:22:00Z

It's connected as in case of FileThing it doesn't have to be path already but it will always be some sort of file name.
I can split it to separate commits for you if it's gonna make review easier ;D

sampsyo · 2021-05-12T15:33:11Z

That would be awesome, if you don't mind!

JuniorJPDJ · 2021-05-12T16:01:46Z

Done!
If possible - I'd like to get your review and merge on #42 first tho.

`MediaFile.as_dict() converts tags to dict usable in for example `MediaFile.update()` or just printing purposes.

JuniorJPDJ · 2021-05-18T00:55:47Z

Rebased on top of master.

sampsyo

A few miscellaneous comments within!

sampsyo · 2021-05-12T21:50:38Z

test/test_mediafile_edge.py

@@ -436,7 +436,6 @@ def test_write(self):
        shutil.copy(src, path)
        mf = mediafile.MediaFile(path)
        mf.read_only_field = "something terrible"
-        mf.path = os.path.join(self.temp_dir, b'test.flac')


I don't quite see why this needs to be deleted.

def test_write(self): src = os.path.join(_common.RSRC, b'empty.flac') path = os.path.join(self.temp_dir, b'test.flac') shutil.copy(src, path) mf = mediafile.MediaFile(path) mf.read_only_field = "something terrible" mf.path = os.path.join(self.temp_dir, b'test.flac') mf.save() self.assertNotIn(self.key, mf.mgfile.tags)

This is a whole function.
path is already set to exactly this path and then handled to mediafile in line:

mf = mediafile.MediaFile(path)

So it was setting mf.path to mf.path ;D
Sooo instead of fixing this code when renaming .path, I just removed it and it still works!

Huh; interesting! I wonder why this was here in the first place.

sampsyo · 2021-05-12T21:50:52Z

mediafile.py

@@ -1518,6 +1549,19 @@ def __init__(self, path, id3v23=False):
        # Set the ID3v2.3 flag only for MP3s.
        self.id3v23 = id3v23 and self.type == 'mp3'

+    @property
+    def filesize(self):


Probably could use a docstring?

It's pretty obvious to me, but if you say it's needed.. ;D

sampsyo · 2021-05-12T21:51:39Z

mediafile.py

@@ -1468,20 +1495,24 @@ class MediaFile(object):
    """Represents a multimedia file on disk and provides access to its
    metadata.
    """
-    def __init__(self, path, id3v23=False):
+    @loadfile()
+    def __init__(self, filething, id3v23=False):


Maybe the docstring needs updating to describe what a filething is?

I'm not sure if I can say what exactly is it without scaring users ;/

mutagen itself just mentions that you can put string or filelike as first argument.
Problem is that technically this first argument is eaten by loadfile decorator and it's not filething from func definition. filething from func definition is namedtuple from mutagen._utils.

So TLDR:
I'm not sure how to document this.
Do you have any idea?

I think I'd just say that filething can either be a string or a file-like object.

sampsyo · 2021-05-12T21:52:24Z

mediafile.py

 # Utility.

+
+def _update_filething(filething):


Also perhaps needs a docstring? Some explanation could help explain why this is necessary.

The problem is I'm not exactly sure why and where exactly closing happens.
Debugging this issue was guessing game.
mutagen was closing my files (only in case of local files, wtf) after reading and I couldn't find any .close() call in whole mutagen code nor using debugger on own proxy file-like.
Anyway - this allows to reopen files in r+ when saving when opened as r on loading.
Proxy file-like weren't closed at all..

I'm not sure what could I write in this docstring what's not written in comment anyway.

I found the reason!
mutagen._util._openfile closes files it opened just after function needing file finishes.

sampsyo · 2021-05-12T21:52:55Z

mediafile.py

+    except UnreadableFileError:
+        # reraise our errors without changes
+        raise


Can you elaborate on why this is necessary now (when it wasn't before)?

__init__ function is now whole wrapped by loadfile decorator, which calls mutagen._util.loadfile through mutagen_call (original loadfile also raises exceptions) so we.. use whole __init__ function through mutagen_call and then call it inside, so exceptions are caught by it two times.

I'm now, after some time, considering removing mutagen_call from there and catching this exceptions myself in decorator.
It probably would be cleaner solution...

It's not cleaner solution as it finished almost as copy-pasta of mutagen_call.
I'll just add one more comment here and leave it as it was in previous version.

sampsyo · 2021-05-12T21:54:07Z

mediafile.py

    except Exception as exc:
        # Isolate bugs in Mutagen.
        log.debug(u'%s', traceback.format_exc())
        log.error(u'uncaught Mutagen exception in %s: %s', action, exc)
        raise MutagenError(path, exc)


+def loadfile(method=True, writable=False, create=False):


One last place where a docstring could be helpful… even just referencing the original loadfile decorator could help explain why this exists.

Ok ;D
Should be pretty easy.

fixes support for filelike objects resolves beetbox#35 Incompatible changes: - renames `path` argument to `MediaFile` to `filename` (compatibility with mutagen) MediaFile can now be created using following scheme: ``` fileobj = io.BytesIO() MediaFile(filename="myfile.mp3") MediaFile(fileobj=myfileobj) ``` but old style ``` MediaFile("myfile.mp3") MediaFile(myfileobj) ``` will also work.

This doesn't have to be path as we are using FileThing to access file - it doesn't even have to be any local file anyway FileThing should have name inside, so we can use it as filename to provide user readable representation of this FileThing to user

JuniorJPDJ · 2021-05-21T22:45:25Z

I think I fixed most of issues you asked about ;D

JuniorJPDJ · 2021-05-22T00:46:38Z

I re-added MediaFile.path as @property - I don't want to completely break backwards compatibility.

It should be compatible with most of existing software now, as far as someone don't set path in their code. I don't see a point of doing this anyway as afaik it didn't change anything - saving still worked on previous file.

For backwards compatibility Also changes `MediaFile.filename` to `@property` - holding reference to string is pointless and it shouldn't be settable.

JuniorJPDJ · 2021-06-04T00:36:19Z

.

sampsyo · 2021-06-27T19:55:16Z

Just returning to this PR after a long delay—is everything in order from your perspective, @JuniorJPDJ? Maybe we should add something to the documentation about this new capability so people know they can use it with open files?

JuniorJPDJ · 2021-06-27T20:42:07Z

mediafile.py

+    def __init__(self, filething, id3v23=False):
+        """Constructs a new `MediaFile` reflecting the provided file.
+        May throw `UnreadableFileError`.
+        First argument can be a string (file path) or filelike object.


There's something about it.
IMO it's enough.

JuniorJPDJ · 2021-06-30T02:19:43Z

IMO it's enough. @up is line describing it
If you think it's not enough - tell me please what exactly would you like to see.

JuniorJPDJ · 2021-07-26T15:22:31Z

bump

sampsyo · 2021-08-17T18:52:15Z

Thanks for your patience! This all looks great. I have updated some documentation and will merge now. ✨

JuniorJPDJ force-pushed the filething branch 2 times, most recently from 724c135 to bb72d66 Compare May 12, 2021 01:46

JuniorJPDJ force-pushed the filething branch from bb72d66 to 40679f7 Compare May 12, 2021 15:57

Add MediaFile.as_dict()

4bacb4f

`MediaFile.as_dict() converts tags to dict usable in for example `MediaFile.update()` or just printing purposes.

JuniorJPDJ force-pushed the filething branch from 40679f7 to 6117853 Compare May 18, 2021 00:12

JuniorJPDJ marked this pull request as ready for review May 18, 2021 00:14

sampsyo reviewed May 18, 2021

View reviewed changes

JuniorJPDJ added 2 commits May 22, 2021 00:36

JuniorJPDJ force-pushed the filething branch from 6117853 to 5f11659 Compare May 21, 2021 22:44

JuniorJPDJ force-pushed the filething branch from 3bcbf00 to 176245d Compare May 22, 2021 00:51

Re-add MediaFile.path

2eeeeb8

For backwards compatibility Also changes `MediaFile.filename` to `@property` - holding reference to string is pointless and it shouldn't be settable.

JuniorJPDJ force-pushed the filething branch from 176245d to 2eeeeb8 Compare May 22, 2021 00:51

JuniorJPDJ commented Jun 27, 2021

View reviewed changes

sampsyo added 3 commits August 17, 2021 14:47

Expand/clarify some docstrings

e920599

Strip some whitespace

e02c1fc

Changelog entry for file-like objects (beetbox#45)

df476c3

sampsyo merged commit e88adb4 into beetbox:master Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File-like objects support #45

File-like objects support #45

JuniorJPDJ commented May 12, 2021

sampsyo commented May 12, 2021

JuniorJPDJ commented May 12, 2021

sampsyo commented May 12, 2021

JuniorJPDJ commented May 12, 2021

JuniorJPDJ commented May 18, 2021

sampsyo left a comment

sampsyo May 12, 2021

JuniorJPDJ May 18, 2021 •

edited

Loading

sampsyo May 19, 2021

sampsyo May 12, 2021

JuniorJPDJ May 19, 2021 •

edited

Loading

sampsyo May 12, 2021

JuniorJPDJ May 19, 2021

sampsyo May 19, 2021

sampsyo May 12, 2021

JuniorJPDJ May 19, 2021

JuniorJPDJ May 21, 2021

sampsyo May 12, 2021

JuniorJPDJ May 19, 2021

JuniorJPDJ May 19, 2021

JuniorJPDJ May 21, 2021

sampsyo May 12, 2021

JuniorJPDJ May 19, 2021

JuniorJPDJ commented May 21, 2021

JuniorJPDJ commented May 22, 2021 •

edited

Loading

JuniorJPDJ commented Jun 4, 2021

sampsyo commented Jun 27, 2021

JuniorJPDJ Jun 27, 2021

JuniorJPDJ commented Jun 30, 2021

JuniorJPDJ commented Jul 26, 2021

sampsyo commented Aug 17, 2021

File-like objects support #45

File-like objects support #45

Conversation

JuniorJPDJ commented May 12, 2021

sampsyo commented May 12, 2021

JuniorJPDJ commented May 12, 2021

sampsyo commented May 12, 2021

JuniorJPDJ commented May 12, 2021

JuniorJPDJ commented May 18, 2021

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuniorJPDJ May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuniorJPDJ May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuniorJPDJ commented May 21, 2021

JuniorJPDJ commented May 22, 2021 • edited Loading

JuniorJPDJ commented Jun 4, 2021

sampsyo commented Jun 27, 2021

Choose a reason for hiding this comment

JuniorJPDJ commented Jun 30, 2021

JuniorJPDJ commented Jul 26, 2021

sampsyo commented Aug 17, 2021

JuniorJPDJ May 18, 2021 •

edited

Loading

JuniorJPDJ May 19, 2021 •

edited

Loading

JuniorJPDJ commented May 22, 2021 •

edited

Loading