Bug 1428000 - Migrate: only annotate affected files #34
Conversation
There was a problem hiding this comment.
I'm a tad unsure on the abspath vs path change. Can you add more detail on that in the commit message? I think I know why it's OK in blame (cwd=root vs not), but not in blame -> migration.
The other question I have is whether we should first collect all the migration contexts, then gather all files, and run blame once?
tools/migrate/migrate-l10n.py
Outdated
| # Annotate legacy localization files used as sources by this migration | ||
| # to preserve attribution of translations. | ||
| files = ctx.localization_resources.keys() | ||
| blame = Blame(localization_dir).main(files) |
There was a problem hiding this comment.
I think we should pass the hglib client to Blame directly. That way, we only have one hg process open instead of ... n + 1 minus whatever gets garbage-collected?
Either in this patch or in a follow-up, I think.
tools/migrate/blame.py
Outdated
| @@ -1,8 +1,10 @@ | |||
| import argparse | |||
| import json | |||
| from os.path import join | |||
There was a problem hiding this comment.
PS: I prefer to have os.path.join explicit rather than importing join into the global namespace. Just import os is good enough. At least that's what more pythonic people have told me to do ;-)
|
Thanks for the review!
In blame data,
Annotating now takes under a second which I think is good enough. The code is cleaner now when annotating happens for each migration context separately. Also, batching blames would only help us in the scenario when we run multiple migrations touching the same files... and it would likely shave off a fraction of a second per locale or so. Let's keep this simple for now and revisit as a potential perf optimization if we hit another bottleneck. |
In `hg annotate` data, `path` is the path relative to CWD and `abspath` is the path relative to the root of the repo. With CWD==root, they're the same and we can remove `abspath` to reduce the number of different types of paths used in the code.
There was a problem hiding this comment.
Thanks, now I get it.
Also OK to only refactor the loop for a single blame if we get to need it. I also wondered that there might be situations where it's slower, aka, you have two distinct migrations with different blame authors, then you check empty changesets in one from the other. We'll see, likely hardly noticable impact.
- Implement Fluent Syntax 0.5.
- Add support for terms.
- Add support for `#`, `##` and `###` comments.
- Remove support for tags.
- Add support for `=` after the identifier in message and term
defintions.
- Forbid newlines in string expressions.
- Allow trailing comma in call expression argument lists.
In fluent 0.6.x the new Syntax 0.5 is supported alongside the old
Syntax 0.4. This should make migrations easier.
`FluentParser` will correctly parse Syntax 0.4 comments (prefixed with
`//`), sections and message definitions without the `=` after the
identifier. The one exception are tags which are no longer supported.
Please use attributed defined on terms instead.
`FluentSerializer` always serializes using the new Syntax 0.5.
- Expose `FluentSerializer.serialize_expression`.
- Fix Bug 1428000 - Migrate: only annotate affected files (#34)
https://bugzilla.mozilla.org/show_bug.cgi?id=1428000