Page MenuHomePhabricator

Implement phutil_console_get_terminal_width for windows
Needs RevisionPublic

Authored by theunreplicated on Sep 30 2015, 9:46 PM.

Details

Summary

See T9444. The regex can't be straightforward because the item in mode con that indicates the columns is system-language-specific.

Test Plan

tested that part in an external file on local webserver

Diff Detail

Repository
rPHU libphutil
Branch
master
Lint
Lint OK
Unit
Unit Test Errors
Build Status
Buildable 8110
Build 9246: [Placeholder Plan] Wait for 30 Seconds
Build 9245: arc lint + arc unit

Event Timeline

theunreplicated retitled this revision from to implement phutil_console_get_terminal_width for windows. The width(number of columns) is being taken by searching for the column count in `mode con` with a regex, that origins from stackoverflow,but a (heavily) modified is used(see discussion in....
theunreplicated updated this object.
theunreplicated edited the test plan for this revision. (Show Details)
theunreplicated added a reviewer: epriestley.

I don't know whether Lint is OK,as described above I modified the arcanist source code in order to be able to commit this, I have no working linters.Otherwise it would always invoke lint without prompt which fails on windows as it tries to build the xhpast linter with the visual c++ by using make commands from the unix world which is totally wrong. But the first time I tried to commit this arcanist prompted me to explain why the linting failed so this could be committed without a successfull lint,afaik I aborted and the next times it would just refuse to commit(Exception:xhpast is broken),which is odd.Deleting arcanist and phutil and redoing the changes by hand again didn'T solve it,so it seems to save settings somewhere in the system, I have no clue where, %programdata%/phabricator doesn'T exist and .arcrc has nothing to do with that.

chad retitled this revision from implement phutil_console_get_terminal_width for windows. The width(number of columns) is being taken by searching for the column count in `mode con` with a regex, that origins from stackoverflow,but a (heavily) modified is used(see discussion in... to Implement phutil_console_get_terminal_width for windows.Sep 30 2015, 10:45 PM
chad updated this object.
chad edited edge metadata.

There's some unnecessary whitespace that should probably be cleaned up - most of the empty lines could probably be removed, and take a look at the bullet points here:
https://secure.phabricator.com/book/phabcontrib/article/php_coding_standards/#spaces-linebreaks-and-in

There should probably also be a unit test that verifies the regex works properly on expected output of mode con to prevent future regressions (possibly test English along with another language).
https://secure.phabricator.com/book/phabcontrib/article/unit_tests/


I'm not sure how to fix xhpast on windows -- I ran into similar issue previously when modifying libphutil for getting debug output. I wasn't looking to make submittable changes however and managed to get past the need for it (by ignoring it?). As for the lint I believe these projects specify in the .arclint at the root of the project that the linter needs to run when creating a diff.

src/console/format.php
191

In this case I would suggest adding a comment with an example output of mode con. It will give a much clearer idea to someone who comes across the code what the regex is attempting to parse. Additionally I would suggest explaining the magic 2.

195

This should probably verify that the value can be parsed as an int or try/catch. Otherwise it could break the existing behavior, and it appears that null is already handled as a return value.

hach-que added inline comments.
src/console/format.php
191

Don't execute programs like this; use Exe Future or the exec passthru function that libphutil provides.

epriestley edited edge metadata.

See feedback.

This revision now requires changes to proceed.Nov 2 2015, 4:44 PM