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

linenoise -> linenoise-ng ? #1703

Merged
merged 3 commits into from Dec 11, 2017
Merged

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Nov 28, 2017

Curiosity killed the @dtzWill, round 471:

In this PR I replaced the vendored linenoise library with the almost-entirely-a-drop-in-replacement fork linenoise-ng.

What motivated this was to improve the completion behavior of nix repl on attribute sets: a good way to see the difference is by entering 'builtins.' and hitting <TAB>.

With linenoise (current): completions are offered one-at-a-time and must be cycled through alphabetically to see if there is an attribute you'd like.

With linenoise-ng (this PR): completions are listed in a "table", like the REPL did when readline was used.

In theory switching to linenoise-ng provides UTF-8 support, but I didn't investigate this.

I can't speak for the linenoise-ng library's suitability one way or the other, but since this seems to work well I thought I'd share in case someone finds it useful :).

The resulting store paths seem comparable in size, but a bit larger: 7940720 before and 7987480 after, ~47Kb (~0.6% increase)

@copumpkin
Copy link
Member

Out of curiosity, why is this "vendored" in in the first place, rather than a quick fetchFromGitHub call that then gets plugged appropriately into the source tree/shell environment?

@bgamari
Copy link
Contributor

bgamari commented Dec 7, 2017

I'll note that the regression in nix repl from nix-repl significantly affects my ability to use Nix: finding my way around nixpkgs has becoming significantly harder with nix 0.12 due to the new completion behavior.

@shlevy
Copy link
Member

shlevy commented Dec 7, 2017

@edolstra Barring any objections I'll merge next week.

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

4 participants