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

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

Details

Reviewers

amckinley

Maniphest Tasks

T13098: Plans: Arcanist toolsets and extensions

Commits

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

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

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 20866
Build 28378: Run Core Tests

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.

src/log/ArcanistLogEngine.php
24–26	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
8	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
8	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