1318960 - Migrate files only when messages change

Status: RESOLVED FIXED

(Localization Infrastructure and Tools :: Fluent Migration, defect)

Description

[Axel Hecht [:Pike]]

Right now, the guard on wether to change a file or not is

    if snapshot.toJSON() == current.toJSON():

We should only change the current file if the AST of the entities change.

It'd also be a nice optimization if we wouldn't toJSON() both ASTs. Given how often we'll run that code in practice, that might be worthwhile.

Comment 1

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #0)
> Right now, the guard on wether to change a file or not is
> 
>     if snapshot.toJSON() == current.toJSON():
> 
> We should only change the current file if the AST of the entities change.

What's the rationale for this requirement?

> It'd also be a nice optimization if we wouldn't toJSON() both ASTs. Given
> how often we'll run that code in practice, that might be worthwhile.

I considered implementing Node.__eq__ but it looked like a lot of work.  I don't think we should be worried about the performance at all tbh.

Comment 2

[Axel Hecht [:Pike]]

(In reply to Staś Małolepszy :stas from comment #1)
> (In reply to Axel Hecht [:Pike] from comment #0)
> > Right now, the guard on wether to change a file or not is
> > 
> >     if snapshot.toJSON() == current.toJSON():
> > 
> > We should only change the current file if the AST of the entities change.
> 
> What's the rationale for this requirement?

That we don't end up with files that only have a license header, for example. Also, as a side effect of that, less commits for many migrations.

Also, forward looking this will be the right indication when doing minimally-invasive edits based on the work here.

> > It'd also be a nice optimization if we wouldn't toJSON() both ASTs. Given
> > how often we'll run that code in practice, that might be worthwhile.
> 
> I considered implementing Node.__eq__ but it looked like a lot of work.  I
> don't think we should be worried about the performance at all tbh.

I'm worried about performance of stuff to be run during merge day, tbh.

Comment 3

[Staś Małolepszy :stas]

https://github.com/projectfluent/python-fluent/commit/73214e4686390dbc1fd068071db1cbb8347c3963 introduced BaseNode.equals which will help avoid serialization to JSON. I already tested it and all tests passed.

Comment 4

[Staś Małolepszy :stas]

Attachment: Pull Request

Comment 5

[Axel Hecht [:Pike]]

I got a couple of questions on the identity check:

What's the right set of ignored_fields? Right now, you're using the default ['span'], I wonder if we should use ['comment','span']?

And, should we really re-serialize on order?

Comment 6

[Staś Małolepszy :stas]

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

> What's the right set of ignored_fields? Right now, you're using the default
> ['span'], I wonder if we should use ['comment','span']?

I don't think so. This is only relevant in the scenario when a message had already been migrated by the localizer manually and the value is the same as in the legacy file but the comment is different than in en-US. I think we should then leave the message as migrated by the localizer.

> And, should we really re-serialize on order?

I don't have a strong opinion about this. We can sort them before the equality check. That said, if subsequent changesets modify even one message, we'll re-serialize the whole file using the order from the en-US template anyways. Might as well do it when messages don't change except for their order. I'm happy leaving the decision to you.

Comment 7

[Axel Hecht [:Pike]]

'k re comments.

As for sorting, I think we should only touch the file when we modify the contents. In that context, yeah, let's sort first, please.

Comment 8

[Staś Małolepszy :stas]

Landed in https://github.com/projectfluent/python-fluent/commit/9ec3db8322b5bd074e506a166caff714c243373a.