Page MenuHomePhabricator

Separate all commit message field parsing out of Differential custom fields
ClosedPublic

Authored by epriestley on Dec 14 2016, 9:38 PM.
Tags
None
Referenced Files
F13060467: D17058.diff
Fri, Apr 19, 5:53 PM
Unknown Object (File)
Thu, Apr 11, 8:48 AM
Unknown Object (File)
Sun, Apr 7, 10:43 AM
Unknown Object (File)
Sat, Mar 30, 3:28 AM
Unknown Object (File)
Fri, Mar 29, 1:57 PM
Unknown Object (File)
Tue, Mar 26, 7:25 PM
Unknown Object (File)
Tue, Mar 26, 2:30 PM
Unknown Object (File)
Tue, Mar 26, 9:47 AM
Subscribers

Details

Summary

Ref T11114. See that task for some discussion.

Overall, Differential custom fields ended up with too many responsibilities. Later work in EditEngine provides a more promising model for achieving modularity with smaller, more consistent components.

In particular, we have some custom fields like DifferentialGitSVNIDField and DifferentialConflictsField which serve only to support the field parser.

This starts pulling commit message responsibilities out of the core list of custom fields and into simpler dedicated parsers.

Test Plan

Created and edited revisions from the CLI. Added a bit of test coverage.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Separate all commit message field parsing out of Differential custom fields.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Dec 14 2016, 10:30 PM
This revision was automatically updated to reflect the committed changes.
joshuaspence added inline comments.
src/applications/differential/parser/DifferentialCommitMessageParser.php
69

Unless I am missing something, this should probably be array $fields

Also, is it intentional that the field name is wrapped in pht(...) but the field aliases are not?

Good catch on the array typehint, I think I just missed that. I'll add it in a followup.

The lack of pht() on the lists of elements is intentional, but tricky / nonfinal. There are two issues:

One issue is that there may be 3 variants in English ("Fixes", "Fixed", "Fix"), but, say, 6 variants in Chinese, 22 variants in Russian, and only 2 variants in French. Ideally we'd have some kind of pht_list(...) construct which allows translators to provide a list of different strings. A better example of this might be if we implement an emoji-suggestor down the road (where you type "boar" and get ๐Ÿ— suggested): an emoji like ๐Ÿ— might have many more names and associations in Russian than in Spanish. If we just pht() each English item in the list, each other language must have exactly that many items. A simple way to build this would be to just pht('a, b, c') and then explode the result on , and trust translators to figure it out. We might end up doing that, or something similar. But it would theoretically be nice to have something a little more formal.

The second issue is that commit messages are ultimately written to the repository and they can't be parsed unambiguously if we don't know which language they were written in. I don't have a solution for that, yet. Probably, putting a magical [@en_US@] marker somewhere in the commit body, I guess? Or a big mess of per-repository options and defaults and such. But if we have a commit message and don't know what language it's written in, we can't parse it: "El Plano de Testo" may mean "Test Plan" in Spanish and "Reviewers" in Korean. This has already caused some issues with the Pirate locale and "Reviewers" being written as "Me Hearties" or something, but we're just kind of faking our way through for now since essentially no one actually wants to write commit messages in a non-English language so far.