1318838 - Make l20n.migrate.transforms.{COPY,REPLACE,PLURAL} subclasses of SOURCE

Status: RESOLVED FIXED

(L20n :: Python Library, defect)

Description

[Staś Małolepszy :stas]

It would be nice to reduce the verbosity of the current transforms AST by making COPY, REPLACE and PLURALS subclasses of SOURCE.  Instead of:

    COPY(
        SOURCE('path', 'key')
    )

We'd then only need to write:

    COPY('path', 'key')

I have a WIP branch of this change and one problem I encountered are lambda functions passed into PLURALS which run on each variant.  Consider:

    FTL.Entity(
        id=FTL.Identifier('delete-all-message'),
        value=PLURALS(
            SOURCE(
                'aboutDownloads.properties',
                'downloadMessage.deleteAll'
            ),
            FTL.ExternalArgument('num'),
            lambda var: REPLACE(
                var,
                {'#1': [FTL.ExternalArgument('num')]}
            )
        )
    )

Here, REPLACE takes `var`, which is a string.  It's consistent with REPLACE(SOURCE(…), …) where the return value of SOURCE is also a string.

Comment 1

[Axel Hecht [:Pike]]

I think it's fine to leave REPLACE out at least for now. I think there's benefit to "use this string, but replace stuff".

REPLACE(COPY(), {}) sounds valid to me?

Comment 2

[Staś Małolepszy :stas]

The idea was to be able to use any kind of transform in the lambda.  At it's simplest, lambda var: COPY(var) should work, too.

COPY(…) returns an FTL.Pattern. REPLACE(COPY(…), …) would need to accept FTL.Pattern as the first argument which again wouldn't work with lambda var: REPLACE(var, …) where `var` is a simple string.

Perhaps the simplest solution is to have convenience transforms called COPY_FROM, REPLACE_FROM, PLURALS_FROM which subclass SOURCE and internally use COPY, REPLACE and PLURALS respectively?

Comment 3

[Axel Hecht [:Pike]]

What if SOURCE and COPY were one merged thing, and REPLACE and PLURALS would convert that to a plain string on-demand? Maybe REPLACE might even work across an AST?

Comment 4

[Staś Małolepszy :stas]

I'm not sure if I understand that.  Can you give a few examples of transforms?

Comment 5

[Axel Hecht [:Pike]]

            FTL.Entity(
                id=FTL.Identifier('delete-all-title'),
                value=COPY(
                        'aboutDownloads.properties',
                        'downloadAction.deleteAll'
                )
            ),
            FTL.Entity(
                id=FTL.Identifier('delete-all-message'),
                value=PLURALS(
                        'aboutDownloads.properties',
                        'downloadMessage.deleteAll'
                    FTL.ExternalArgument('num'),
                    lambda var: REPLACE(
                        var,
                        {'#1': [FTL.ExternalArgument('num')]}
                    )
                )
            ),

Now, internally, PLURALS would have a FTL.Pattern from downloadMessage.deleteAll, but would () that to a string before splitting it and passing it in to the lambda.

The other thing I pondered was if we can do

REPLACE(
               PLURALS(
                        'aboutDownloads.properties',
                        'downloadMessage.deleteAll'
                    FTL.ExternalArgument('num'),
                    lambda var: var
               ),
    {'#1': [FTL.ExternalArgument('num')]}
)

which could then iterate over all TextElement nodes in the ast and replace inside them.

Comment 6

[Staś Małolepszy :stas]

There are two separate problems that we're discussing in this bug:

  1) COPY(SOURCE(path, key)) is too verbose.

  2) Transforms should take Patterns not strings as arguments, which will allow FTL to FTL migrations.

I suggest we focus on 1) right now and I'll file a follow-up for 2) which is an internal change and shouldn't affect the migration files.

So this bug is now about taking a string value vs. taking a (path, key) tuple.  In code that would be:

  COPY('foo')
  COPY(SOURCE('path', 'key'))

vs.

  COPY('path', 'key')

In comment 0 I made a point about how PLURALS' lambdas don't fit well into this idea because they work on values rather than (path, key) tuples.

We could special-case the REPLACE transform for this, but as always, I'd prefer to err on the side of consistency.  I'd like to go ahead and implement the idea from comment #2:

> Perhaps the simplest solution is to have convenience transforms called
> COPY_FROM, REPLACE_FROM, PLURALS_FROM which subclass SOURCE and internally
> use COPY, REPLACE and PLURALS respectively?

This introduces a good separation of concerns:  a FOO transform works with a value and requires SOURCE to be used; FOO_FROM takes a (path, key) tuples and abstracts the explicit SOURCE call away.

May I go ahead with this plan?

Comment 7

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #6)
> There are two separate problems that we're discussing in this bug:
> 
>   1) COPY(SOURCE(path, key)) is too verbose.
> 
>   2) Transforms should take Patterns not strings as arguments, which will
> allow FTL to FTL migrations.
> 
> I suggest we focus on 1) right now and I'll file a follow-up for 2) which is
> an internal change and shouldn't affect the migration files.

I filed bug 1320060.

Comment 8

[Axel Hecht [:Pike]]

'k. Looking at migrations in the tests a bit, the COPY name rubs me wrong a bit, in particular when it's just inserting text. How about LITERAL and LITERAL_FROM?

Another detail I noticed while staring at the code is that legacy-entity decoding should probably done in the SOURCE part and not the COPY/LITERAL part?

Comment 9

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #8)
> 'k. Looking at migrations in the tests a bit, the COPY name rubs me wrong a
> bit, in particular when it's just inserting text. How about LITERAL and
> LITERAL_FROM?

I was just going to suggest this very change :)  On my WIP branch it's called TEXT, but LITERAL sounds good, too.

> Another detail I noticed while staring at the code is that legacy-entity
> decoding should probably done in the SOURCE part and not the COPY/LITERAL
> part?

Yeah, this make sense to me.  Although looking at the code, this change will mostly affect the docstring, right?

Comment 10

[Axel Hecht [:Pike]]

(In reply to Staś Małolepszy :stas from comment #9) 
> Yeah, this make sense to me.  Although looking at the code, this change will
> mostly affect the docstring, right?

Yeah.

Comment 11

[Staś Małolepszy :stas]

Attachment: Pull request

Hi Axel -- in this PR I implemented LITERAL_FROM, REPLACE_FROM and PLURALS_FROM.

For legacy translations, do you think we should keep c-l's Entity objects in the MergeContext and only .val them in SOURCE?  Right now, we .val right away when we add_localization()

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 8814385 [details] [review]
Pull request

Only one nit on the PR, r=me with that.

I do think that the migrations read a lot nicer now, so that's sweet.

Comment 13

[Axel Hecht [:Pike]]

(In reply to Staś Małolepszy :stas from comment #11)
> For legacy translations, do you think we should keep c-l's Entity objects in
> the MergeContext and only .val them in SOURCE?  Right now, we .val right
> away when we add_localization()

Because we make tons of claims about unescaping in the transforms, but actually unescape when we add_localization? Interesting question. Don't feel strongly about that, I think.

Comment 14

[Staś Małolepszy :stas]

PR landed in https://github.com/l20n/python-l20n/commit/eb2174a17ff55615049d41c0a89c2b31d266532c

(In reply to Axel Hecht [:Pike] from comment #13)

> Because we make tons of claims about unescaping in the transforms, but
> actually unescape when we add_localization? Interesting question. Don't feel
> strongly about that, I think.

Yes, that was rationale exactly.  We can fix that later if it bites us.