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.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 4:59 PM
Unknown Object (File)
Feb 3 2024, 7:30 PM
Unknown Object (File)
Jan 8 2024, 5:31 PM
Unknown Object (File)
Jan 4 2024, 4:11 PM
Unknown Object (File)
Dec 31 2023, 8:10 PM
Unknown Object (File)
Dec 27 2023, 11:38 PM
Unknown Object (File)
Dec 22 2023, 12:34 PM
Unknown Object (File)
Dec 22 2023, 12:25 AM
Subscribers
None

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
Lint Not Applicable
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 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 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 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*