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.

Sign up for GitHub

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

Jump to bottom

Address #166: Set history file max size from inputrc via JLine #183

Merged
merged 2 commits into from Jul 19, 2018
Merged

Address #166: Set history file max size from inputrc via JLine #183

merged 2 commits into from Jul 19, 2018

Conversation

timmc-bcov
Copy link
Contributor

Set history file max size based on inputrc, using newer jline2 to support this configuration.

Please let me know if there are any tests you would like to see.

@timmc-bcov
Copy link
Contributor Author

OK, this is failing because there's no .inputrc, which I find surprising but I guess not all that surprising. Might be a jline2 bug. -.-

@trptcolin
Copy link
Owner

I have an inputrc locally and was able to reproduce the failures - I see via lein trampoline run that there's a stack trace on startup (unfortunately it's missing from these test outputs because it's not asserting against @fake-err being empty - that might be a good addition to all these integration tests). Here's what I get:

NumberFormatException null
        java.lang.Integer.parseInt (Integer.java:542)
        java.lang.Integer.parseInt (Integer.java:615)
        reply.reader.simple-jline/default-history-size (simple_jline.clj:76)
        reply.reader.simple-jline/setup-console-reader (simple_jline.clj:98)
        reply.reader.simple-jline/get-input-line (simple_jline.clj:131)
        clojure.lang.Atom.swap (Atom.java:37)
        clojure.core/swap! (core.clj:2232)
        reply.reader.simple-jline/safe-read-line (simple_jline.clj:170)
        reply.reader.simple-jline/safe-read-line (simple_jline.clj:177)
        clojure.core/apply (core.clj:626)
        clojure.core/partial/fn--4228 (core.clj:2468)
        reply.eval-modes.nrepl/run-repl (nrepl.clj:133)

So I think it just needs a failsafe where if "history-size" isn't present, it doesn't try to do the NPE-causing Integer/parseInt - maybe some-> instead of ->?

@timmc-bcov
Copy link
Contributor Author

Ah, thanks! That explains why I was having trouble isolating this error. I'll put in a fix.

@timmc-bcov
Avoid various .inputrc failure modes
68fdeef 
- missing history-size declaration
- unparseable history-size value

Also assert that stderr is empty in various integration tests, which
came up while testing this.
timmc-bcov
timmc-bcov commented Jul 18, 2018
View reviewed changes
spec/reply/integration_spec.clj
(should-contain #"Unmatched delimiter.+\n.+\n.+Unmatched delimiter" (str @fake-out)))

(it "does not print an error when given empty input lines"
(main/launch-standalone {:input-stream
(java.io.ByteArrayInputStream.
(.getBytes "\n\n\n"))
:output-stream @fake-out})
(should= "" (str @fake-err))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this addition actually obviates these existing should-not-contain checks on stdout, or if those errors might really appear on stdout if a bug is present.

Copy link
Owner

Choose a reason for hiding this comment

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

Hrm, great question - I suspect you're right that these should've been checks against fake-err. In any case, non-blocker. Thanks!

@timmc-bcov
Copy link
Contributor Author

OK, this works better, and I fixed an additional issue around possibly malformed history-size values (non-numbers) in .inputrc files.

@trptcolin trptcolin merged commit 4816d9f into trptcolin:master Jul 19, 2018
@trptcolin
Copy link
Owner

Thanks!! 🎉

@timmc-bcov timmc-bcov deleted the 166-max-history-size branch July 20, 2018 16:48
@timmc-bcov
Copy link
Contributor Author

And thank you!

@timmc-bcov timmc-bcov mentioned this pull request Jul 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trptcolin trptcolin trptcolin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@timmc-bcov @trptcolin