Page MenuHomePhabricator

[Wilds] Shove the logging stuff into a bit of an abstraction before it gets out of control
ClosedPublic

Authored by epriestley on Sep 20 2018, 10:45 PM.

Details

Summary

Ref T13098. The logging stuff is starting to get a little sketchy, so wrap it in several layers of class-based indirection before it can escape its cage.

This at least gives us an actual API and structure to work with later instead of scattering a lot of fprintf(STDERR, ....) all over the place.

Test Plan

Ran a few commands, got slightly more sensible/consistent logging out of them.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 20 2018, 10:45 PM
Harbormaster failed remote builds in B20866: Diff 47054!
epriestley requested review of this revision.Sep 20 2018, 10:45 PM
epriestley marked an inline comment as done.
epriestley added inline comments.
src/log/ArcanistLogEngine.php
25–27

This should actually be (STDERR, "%s", ...); I fixed it locally.

amckinley accepted this revision.Sep 21 2018, 8:07 PM
amckinley added inline comments.
src/config/source/ArcanistConfigurationSource.php
27

Any chance of moving this pht() call into the LogEngine? Or is it important that pht() invocations happen near a string's definition to help with translations? (I haven't looked under the hood at pht at all).

src/log/ArcanistLogEngine.php
9

Type-hint this as a bool? I feel like I've asked this question before, but I don't remember the answer.

support/ArcanistRuntime.php
34

Assuming this is the saner version of return 77;.

39

lolwut

This revision is now accepted and ready to land.Sep 21 2018, 8:07 PM
epriestley marked 3 inline comments as done.Sep 21 2018, 8:36 PM
epriestley added inline comments.
src/config/source/ArcanistConfigurationSource.php
27

Yeah, pht(...) needs to be right at the source of the string. The string extractor (which generates a list of strings which need translation, for translators) works by looking for pht(<some string>) in the source code using static analysis.

We could theoretically make writeWarning(...) extract strings like pht(), but I think we're better off avoiding that in cases where the argument isn't super compelling.

Some of the workflows I'm touching right now are very user-facing, I think we won't have quite as much logging happening in the real workflows (diff, branch, etc) although the next diff I'm working on now (shell-complete) is now like 95% user interaction.

src/log/ArcanistLogEngine.php
9

No typehints for scalar types (bool, int, ...) until PHP7.

support/ArcanistRuntime.php
34

Yeah. I noted that we no longer have random crazy behavior in T13203 OwO *drops spork*

This revision was automatically updated to reflect the committed changes.