Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alpine reports a width of 0 for process.stdout.getWindowSize[0] #6

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jan 25, 2016

When running through docker exec the Alpine OS reports process.stdout.getWindowSize[0] = 0, this in turn breaks the upstream inquirer dependency like so:

https://gist.github.com/bcoe/6bc16d0dbe5190b8d302

It seems appropriate to instead use the default value if a terminal width of 0 is reported.

knownasilya pushed a commit that referenced this pull request Jan 26, 2016
alpine reports a width of 0 for process.stdout.getWindowSize[0]
@knownasilya knownasilya merged commit 45ae4c6 into knownasilya:master Jan 26, 2016
@knownasilya
Copy link
Owner

Thoughts on semver for this change? It almost seems like a breaking change..

@bcoe
Copy link
Contributor Author

bcoe commented Jan 26, 2016

@knownasilya great question, what do you think @SBoudrias? seems like we could go one of two ways:

  1. stick with having upstream libraries opt to use a default value if process.stdout.getWindowSize() reports 0.
  2. perhaps call this a major change as @knownasilya suggests, and change the contract of the API slightly.

From my point of view, 0 being returned felt like unexpected behavior, at the same time it's what process.stdout.getWindowSize() reported.

@SBoudrias
Copy link
Collaborator

Yeah, having 0 returned looks like a bug to me too.

@knownasilya can you expand your though as to why this would be a breaking change?

@knownasilya
Copy link
Owner

@SBoudrias mainly because consumers have expected it to be zero if all fallbacks fail (or return 0), but maybe 0 is never a desired value so it would technically be an enhancement.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 26, 2016

@knownasilya thanks for being so receptive to a patch 👍

@SBoudrias
Copy link
Collaborator

@knownasilya oh, I was confused because I though that was a discussion on the Inquirer repo.

So yeah, that looks like major bump worthy for cli-width.

knownasilya pushed a commit that referenced this pull request Jan 26, 2016
Use default if result is 0, see #6
@knownasilya
Copy link
Owner

Released as 1.1.1

@knownasilya
Copy link
Owner

So yeah, that looks like major bump worthy for cli-width.

Too late ;) I guess we'll see if anyone complains.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 26, 2016

@knownasilya but semver!

I think you'll probably be okay, it looks like cli-width's dependencies are mostly inquirer, and forks of inquirer:

screen shot 2016-01-25 at 4 36 09 pm

@knownasilya
Copy link
Owner

Semver is very subjective :trollface:

@knownasilya
Copy link
Owner

@bcoe I can add you as a collaborator so we can share the blame 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants