477987 - [silme] improve encoding handling

Status: RESOLVED FIXED

(Mozilla Localizations :: Infrastructure, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

First, I modified get_source to return without encoding in case there is no fallback, no encoding and no char detector.

This scenario is important for Blobs. Imagine a jpeg file that we read as a Blob and then try to save.

Without this change it'll try to read it as utf8 or raise exception.

I'm not confident it's the best solution. Maybe we should split reading files for Blob and for L10nObject instead?

Next is similar change for writing.

Currently it'll write as utf8 even files like JPEG.

Once again, I'm not super-confident its the best possible solution.

Last change switches reading packages to read Blobs always with source. It makes sense cause there's no other goal in reading Blobs than to get the source of it.
It would be much more interesting to be able to select not to read source even for Blobs (for case scenario when I'm just reading package, not writing, and I don't want to present Blobs anyhow. Blobs are usually big. it would save time and memory not to read them) but that would require probably another parameter to get_l10npackage.

Adrian, let me know what you think about it

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch v1

Comment 2

[Adrian Kalla [:adriank]]

                 except Exception:
-                    l10npackage.objects[elem] = cls.get_blob(p2, source=source)
+                    l10npackage.objects[elem] = cls.get_blob(p2, source=True)
                 else:
                     if object_type=='object':
-                        l10npackage.objects[elem] = cls.get_blob(p2, source=source)
+                        l10npackage.objects[elem] = cls.get_blob(p2, source=True)
                     elif object_type=='entitylist':

I think, that always reading the source of the Blob is a bad idea: it slows things really down and it seems to be unnecessary:
-get_l10npackage() has the optional parameter "source", so you are able to force to read the source anyway
-we could try to read the source during a write attempt (if it wasn't read before)
What do you think about that?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

I don't like the concept of combining write and read because I want to keep ability to transport L10nPackage as a fully "reproduceable" unit for example over the network.

So I'd much more prefer ability to read Blob's source always when you want to create a package that you will want to write.

The problem is that for Blob to be writable you *need* source. For others, not.

So I'm thinking about doing sth like this:

if source is True, get sources
if source is False, don't
is source is undefined - get source for Blob/EntityList and ignore for L10nObject.

How does that sound to you?

Comment 4

[Adrian Kalla [:adriank]]

> if source is True, get sources
> if source is False, don't
> is source is undefined - get source for Blob/EntityList and ignore for L10nObject.


Sounds good

Comment 5

[Adrian Kalla [:adriank]]

wait:

> is source is undefined - get source for Blob/EntityList and ignore for L10nObject.

Why for EntityLists? To write an entitylist we don't need the source, do we?

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch, v2

updated patch.

split functions write_to_file and write_to_file_with_encoding and improve source handling on read

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch, v3

improved patch. As Adrian noted on IRC, entitylist should use the same pattern as l10nobject, not blob.

Comment 8

[Adrian Kalla [:adriank]]

Comment on attachment 361980 [details] [diff] [review]
patch, v3

>-        cls.write_to_file(string, os.path.join(path, object.id), encoding)
>+
>+        if isinstance(object, Blob):
>+            cls.write_to_file(string,
>+                              os.path.join(path, object.id),
>+                              encoding)

write_to_file doesn't have the attribute "encoding"... 

>+        else:
>+            cls.write_to_file_with_encoding(string,
>+                                            os.path.join(path, object.id))
>         return True

...but write_to_file_with_encoding has it, and needs it here.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch, v3.1

good catch. thanks!

3.1 with this updated

Comment 10

[Adrian Kalla [:adriank]]

Comment on attachment 361983 [details] [diff] [review]
patch, v3.1

looks good now. Thanks

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

commited, thanks