Complete functions in filesystem.py

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Roland Steiner]

E.g., there exists a function open_text_file_for_writing(), but not the corresponding open_text_file_for_reading().

[Roland Steiner]

Created attachment 98902 [details]
Patch

Patch probably also needs some new tests, but 'testing the waters' first... ;)

[Roland Steiner]

I should add that this is largely similar to the patch in https://bugs.webkit.org/show_bug.cgi?id=63419 , except without the 'encoding' parameters, plus a bit more polishing.

The main reason for this patch is to provide functions needed in https://bugs.webkit.org/show_bug.cgi?id=63527 .

[Tony Chang]

Comment on attachment 98902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98902&action=review

> Tools/Scripts/webkitpy/common/system/filesystem.py:232
> +    def open_text_file_for_appending(self, path):
> +        """Returns a file handle suitable for appending to."""
> +        return codecs.open(path, 'a', 'utf8')

Do you need all these methods?  I think we should only add the functions you need.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:340
> +    def __init__(self, fs, path):
> +        WritableBinaryFileObject.__init__(self, fs, path)

I believe this will be done implicitly even if you don't declare __init__ here.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:373
> +    def __init__(self, fs, path):
> +        ReadableBinaryFileObject.__init__(self, fs, path)

This too.

[Roland Steiner]

(In reply to comment #3)
> Do you need all these methods?  I think we should only add the functions you need.

Good question ;) I won't be needing the 'append' functions, but otherwise I can't say yet (have to work my way through the rest of the project files first). OTOH I felt it's nice to have a complete set without having to extend it in the future.

I guess I'll leave this as a "work-in-progress" patch and update it once I can narrow it down.

[Tony Chang]

Comment on attachment 98902 [details]
Patch

Sounds good to me.  I'm going to remove the r? flag to get it out of the review queue.

[Roland Steiner]

Created attachment 99927 [details]
patch, reduced

Somewhat reduced patch, removed binary file functions that I probably won't be using

[Roland Steiner]

Created attachment 99935 [details]
patch, reduced

Forgot to include unit test in last patch.

[Tony Chang]

Comment on attachment 99935 [details]
patch, reduced

View in context: https://bugs.webkit.org/attachment.cgi?id=99935&action=review

Mostly just some style nits.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:-317
> -        if path not in self.fs.files or not append:

Why is the check for 'path not in self.fs.files' no longer needed?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:340
> +    def __init__(self, fs, path):
> +        WritableBinaryFileObject.__init__(self, fs, path)

Nit: I don't think we need this, it's called implicitly.

> Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:149
> +        unicode_text_string = u'ŪnÄ­cÅde̽'

It might be safer to have this be unicode escaped (e.g., u'\uXXXX\uXXXX') rather than trying to inline directly.  I think we expect all our python files to be utf8.

[Dirk Pranke]

Comment on attachment 99935 [details]
patch, reduced

View in context: https://bugs.webkit.org/attachment.cgi?id=99935&action=review

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:-317
>> -        if path not in self.fs.files or not append:
> 
> Why is the check for 'path not in self.fs.files' no longer needed?

I think the 'path not in self.fs.files' part was just wrong and should never have been there. When it was there, if the file already existed (and had content), that content would've been preserved, which I think is probably the wrong thing to do in most if not all cases.

[Roland Steiner]

Created attachment 100067 [details]
Patch

Escaped Unicode strings, using super() for initialization.

[Roland Steiner]

(In reply to comment #8)
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:-317
> > -        if path not in self.fs.files or not append:
> 
> Why is the check for 'path not in self.fs.files' no longer needed?

AFAICT that was to make sure not to clobber the file contents if 'append' was True. Without the appending case we can always clobber the file contents. And 'append' never was True anyways...
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:340
> > +    def __init__(self, fs, path):
> > +        WritableBinaryFileObject.__init__(self, fs, path)
> 
> Nit: I don't think we need this, it's called implicitly.

I personally hate having to go hunting for constructor parameters in super-classes, so I would prefer if I could leave it in. Changed the call to use super() in the new patch, though, which should be a bit more idiosyncratic.

> > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:149
> > +        unicode_text_string = u'ŪnÄ­cÅde̽'
> 
> It might be safer to have this be unicode escaped (e.g., u'\uXXXX\uXXXX') rather than trying to inline directly.  I think we expect all our python files to be utf8.

Heh, who says it wasn't? ;) But I do agree - escaped the string and the original where I copied it from in the new patch.

[Tony Chang]

Comment on attachment 100067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100067&action=review

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:341
> +    def __init__(self, fs, path):
> +        super(WritableTextFileObject, self).__init__(fs, path)

I think super is worse :)  If you insist on having __init__, let's change this back to WritableTextFileObject.__init__(self, fs, path).

I don't like the pass through constructors because if you add a param to the base class, you have to find and fix all the subclasses.  It's easy to miss some because it's not a compile error.  But calling .__init__ explicitly is OK too.

[Roland Steiner]

Committed r90702: <http://trac.webkit.org/changeset/90702>

[Roland Steiner]

(In reply to comment #12)
> I think super is worse :)  If you insist on having __init__, let's change this back to WritableTextFileObject.__init__(self, fs, path).
> 
> I don't like the pass through constructors because if you add a param to the base class, you have to find and fix all the subclasses.  It's easy to miss some because it's not a compile error.  But calling .__init__ explicitly is OK too.

I'm not that set on either form, so I just removed it upon commit ;).

[Adam Barth]

Comment on attachment 100067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100067&action=review

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:341
>> +        super(WritableTextFileObject, self).__init__(fs, path)
> 
> I think super is worse :)  If you insist on having __init__, let's change this back to WritableTextFileObject.__init__(self, fs, path).
> 
> I don't like the pass through constructors because if you add a param to the base class, you have to find and fix all the subclasses.  It's easy to miss some because it's not a compile error.  But calling .__init__ explicitly is OK too.

Or just omit the __init__.  That's what do usually.