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

update linenoise #16451

Closed
wants to merge 2 commits into from
Closed

update linenoise #16451

wants to merge 2 commits into from

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 23, 2020

fetch the upstream updates

based on this commit

antirez/linenoise@97d2850

@@ -485,6 +499,8 @@ void refreshShowHints(struct abuf *ab, struct linenoiseState *l, int plen) {
if (bold == 1 && color == -1) color = 37;
if (color != -1 || bold != 0)
snprintf(seq,64,"\033[%d;%d;49m",bold,color);
else
seq[0] = '\0';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to have a small fix here

@ringabout

This comment has been minimized.

@ringabout
Copy link
Member Author

Yeah, is this module rarely used?

@Araq
Copy link
Member

Araq commented Dec 23, 2020

Please rewrite all the C code in Nim. We don't need more "static" variables, it's a desaster for multi-threading.

@timotheecour
Copy link
Member

timotheecour commented Dec 23, 2020

Please rewrite all the C code in Nim

That said, I kind of agree with the need for rewrite, although it's a bit unfair to dump it all on this PR's author. Long term this is absolutely the way to go though, for several reasons:

  • https://github.com/antirez/linenoise is stable, not getting much updates/new features
  • it's still very small
  • it has multi-threading issues as noted
  • a rewrite in nim would be much smaller than the already small C code, and would allow us to add more features
  • there are no dependencies other than standard ones

I had actually done exactly that in D some time ago, and it was not hard, and it did allow implementing more features easily in a more high-level langauge.

In nim, this could also be done in multiple stages instead of a one-time conversion, thanks to {.emit.}.

Yeah, is this module rarely used?

no, it's useful anytime you need interactive CLI input in a program (eg for a repl, eg inim)

@ringabout
Copy link
Member Author

I could port them when I have free time.

I will also consider porting a better and modern one:
https://github.com/AmokHuginnsson/replxx

@ringabout ringabout closed this Dec 24, 2020
@ringabout
Copy link
Member Author

It seems that there are already two Pure Nim version of linenoise
https://github.com/jangko/nim-noise
https://github.com/de-odex/linenoise-nim

@timotheecour
Copy link
Member

timotheecour commented Dec 24, 2020

@Araq @xflywind instead of rewriting something that already exists in nimble (in multiple packages in fact), how about this plan:

nimble install noise # suppose it has a `linenoisecompat` module with same interface as std/linenoise
nim --path:std/linenoise:pkg/noise/linenoisecompat main
  • koch boot can itself do: nimble install noise and we can add --path:std/linenoise:pkg/noise/linenoisecompat to nim/config/config.nims, which is overridable in user config

then it all works, and even nim secret can keep working with linenoise without users having to change anything; but users can override linenoise with another nimble package that offers a compatibility module, eg nim-noise, linenoise-nim etc.

  • we can then deprecate lib/wrappers/linenoise/linenoise.nim and after a deprecation period remove lib/wrappers/linenoise/*

note

this approach generalizes to other use cases (there's nothing specific about linenoise) where we're better off reusing nimble packages than re-inventing (poorly) the wheel in stdlib, thus avoiding duplicating effort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants