Page MenuHomePhabricator

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

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



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

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

Event Timeline

epriestley created this revision.Sep 20 2018, 10:45 PM
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.

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

amckinley accepted this revision.Sep 21 2018, 8:07 PM
amckinley added inline comments.

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).


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


Assuming this is the saner version of return 77;.



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.

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.


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


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.