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

ClassCastException when passing --port option #198

Merged
merged 1 commit into from
May 19, 2019
Merged

ClassCastException when passing --port option #198

merged 1 commit into from
May 19, 2019

Conversation

glts
Copy link
Contributor

@glts glts commented May 19, 2019

Commit 7fe67e4 introduced a regression where REPL-y will throw an exception when it gets passed the --port command-line argument. The exception is easily reproducible with the command

clojure -Sdeps '{:deps {reply {:mvn/version "0.4.3"}}}' -m reply.main --port 3333

This breaks clients of the REPL-y command-line API such as Leiningen, see technomancy/leiningen#2572.

The proposed change restores the previous behaviour as faithfully as possible. reply.eval-modes.nrepl/main will again accept both string and integer for :port.

Fixes a regression introduced in 7fe67e4.
@trptcolin
Copy link
Owner

Thanks! 🎉

@trptcolin trptcolin merged commit 908fafb into trptcolin:master May 19, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented May 19, 2019

The proposed change restores the previous behaviour as faithfully as possible. reply.eval-modes.nrepl/main will again accept both string and integer for :port.

I still think that's not the right place to fix this, but I guess that's not a big deal. IMO the conversion should be done when the command-line arguments are read. Pushing this down makes the code slightly more complex than it needs to be.

@bbatsov
Copy link
Collaborator

bbatsov commented May 19, 2019

(to be more concrete - I think the command-line params should be normalized before launch is ever called)

@glts
Copy link
Contributor Author

glts commented Aug 22, 2019

Belated thank you for the merge …!

Are you planning to cut a release some time soon? Would be cool to pull this into Leiningen, to fix the related issue. No pressure though.

@trptcolin
Copy link
Owner

Done - thanks for the nudge! 🎉

@glts
Copy link
Contributor Author

glts commented Aug 22, 2019

Done and done, thank you very much.

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.

3 participants