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

Refactored cmdline for reusability nrepl/nrepl#108 (first cut) #116

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
4 participants
@eccentric-j
Copy link
Contributor

commented Jan 6, 2019

Features

  • Addresses issue #108
  • Splitting up large cmdline functions into a smaller, reusable API

API Functions

name Description
args->cli-functions Processes CLI args into options map with keywordized keys.
help-command Displays help copy to user and exits program
version-command Displays version info and exists program
get-conn-options Return map of resolved options specific to connecting to a running nREPL server.
get-server-options Return map of resolved options specific to starting an nREPL server. Also reuses get-conn-options.
connect-command Connects to a running nREPL server
ack-server Acknowledges an existing nREPL server running on another port (not a command)
print-connection-header Prints the nREPL server connection string that clients like cider parse
save-port-file! Writes the server port to a file and deletes it when the program closes
interactive-repl-or-sleep Runs an nREPL client against the created nREPL server. Corresponds to --interactive CLI flag.
server-command Server pipeline to create an nREPL server and optionally start an interactive repl.
dispatch-commands Simple function to dispatch major commands.
print-repl-intro Displays interactive repl help content.

Checklist

NOTE: Since we decided to discuss the API shape first it's subject to change therefore writing tests at this point would be creating more work for myself. Second, I'm not adding functionality to the cmdline ns so it is still covered by the functional tests that exist for it.

  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the changelog(that only applies to user-visible changes)
  • Ran cljformat
  • Ran eastwood
  • Generated docs

Next Steps

Let's discuss the flow, signatures, and names of the proposed API functions and decide what would be best suited for a consistent API across time. After, I'm willing to write tests for all the functions I've created but would like some advice on resolution. I'm also willing to write tests for related functions I did not write, within reason, and perhaps some insight as to the priority.

Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2019

One more thing to consider - it'd be nice if we made it easy to specify just a different REPL function. That would make it easy for people to select something like REPL-y or maybe another REPL based on rebel-readline.

I guess that'd be as simple as making it possible to pass a var via the command-line or the config file.

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 6, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 6, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 6, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2019

One more thing - now that we're making this API we should also add a few unit tests for the various building blocks.

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

One more thing to consider - it'd be nice if we made it easy to specify just a different REPL function. That would make it easy for people to select something like REPL-y or maybe another REPL based on rebel-readline.

I guess that'd be as simple as making it possible to pass a var via the command-line or the config file.

That is a great idea. Is there any source I should look at to get a sense of how to get the option and the default from the config? Lastly, should I look at how leiningen incorporates repl-y to get a sense for how to setup an external repl lib with nREPL?

One more thing - now that we're making this API we should also add a few unit tests for the various building blocks.

I commented on that in the updated pull request copy. If we're in a good spot with the API I'll add more tests to the API functions for that reason :) I just figure everything is up in the air for now and didn't want to have to do double work in this drafting stage.

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

However, is adding the custom REPL option something we want to tackle as part of this PR\feature set or something that can come in separately?

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2019

That is a great idea. Is there any source I should look at to get a sense of how to get the option and the default from the config? Lastly, should I look at how leiningen incorporates repl-y to get a sense for how to setup an external repl lib with nREPL?

There are already such examples in the current cmdline code - e.g. the code related to the hander or the middleware. As for REPL-y - it's really trivial https://github.com/nrepl/lein-nrepl/blob/master/src/leiningen/nrepl/core.clj#L38

I commented on that in the updated pull request copy. If we're in a good spot with the API I'll add more tests to the API functions for that reason :) I just figure everything is up in the air for now and didn't want to have to do double work in this drafting stage.

Yeah, I figured as much. That makes perfect sense. Once you've addressed all of my feedback I'll do a deeper dive and afterwards you can tackle the tests.

However, is adding the custom REPL option something we want to tackle as part of this PR\feature set or something that can come in separately?

Whatever you prefer. It's pretty small compared to the overall scope of the PR.

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 6, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 6, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 6, 2019

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

@bbatsov The tests are failing because of the stdout-stderr test in test/clojure/nrepl/sanity_test.clj#L80 which also failed in master about 2 commits ago.

I am fairly certain nothing I wrote between today and yesterday should affect that. Is that something I should look into more or is it some kind of edge case?

Anyway the changes you requested are done. I'm going to look into setting up that REPL option (thanks for the refs) and get back to you when I have an update on that. Does it make sense for you to dive deeper or do you want to wait until I get that feature done?

@arichiardi
Copy link
Contributor

left a comment

This patch is a great refactoring. Things seem much more testable and composable.

Apart from the two names, I am curious to see where the !s are going because I personally would put them at the end of side effectful things like ack-server...I know this is kind of a blurred topic though...

All in all, good job!

Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated
@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

@arichiardi Thanks for the feedback. 😄

My understanding of ! is that it's used to denote side-effects that write run-time state for instance swap!, reset!, and set! compared to functions that just return values such as rand and time. So that's why I only used it on save-port-file! but I see as much validity in adding it to functions like ack-server like you suggested as much I see validity in removing it from save-port-file! since it's not writing state that affects run-time anyway.

I would like to defer to you to decide what is best.

@arichiardi

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Oh that's a tough hot potato you pass me 😄

I guess we agree that values are without. Long time ago Clojurists were saying that a ! goes to all the fns that are not safe in the STM machinery, aka not idempotent - maybe?
I don't know if that's still a valid statement and in particular I remember having seen a Stuart Sierra articles around denying that...cannot find it now.

In my daily job I try to put it where there is anything side effectful really, does not matter if it's only a read or a write. My vote goes to have it in both and in every place where we write to desk or to socket.

However not really a big deal in case there are adverse opinions.

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 7, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 7, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 7, 2019

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

All previous comments have been resolved. Though I'm still unsure why that sanity test that was failing is now passing. Kinda seems like a race condition there?

Show resolved Hide resolved doc/modules/ROOT/pages/ops.adoc Outdated
Show resolved Hide resolved src/clojure/nrepl/cmdline.clj Outdated

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 8, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 8, 2019

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

I'll remove the ! then, as much as @arichiardi and I like it in our personal work, if it doesn't match the conventions of the rest of the codebase and doesn't reflect something you two would like to do going forward then I should match the way it is.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

I highly doubt anyone would ever use this api in a concurrent context anyways. :-)

I'm ok with the current state of the API, so you can add some tests, squash and rebase.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 10, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 10, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 10, 2019

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

  • Write unit tests for cmdline API functions
  • Add support for :repl-fn option (see lein-reply link referenced above)
  • Add relevant unit tests for repl-fn option
  • Run lein test-all
  • Run lein with-profile cljfmt cljfmt check
  • Run lein with-profile eastwood eastwood
  • Run lein docs
  • Squash commits
  • Rebase

Thanks a ton for your support thus far. My last question for now is what's the difference between squashing the commits and rebasing?

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Squashing commits is the act of combining related commits together, so you can get a cleaner git history. Rebasing is applying commits after a certain commit (to avoid the branching that comes from merging without the ability to do a fast forward merge). Hopefully this makes sense.

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@eccentric-j Ping :-)

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

In progress of writing tests. I have a few done but there's a tight deadline at work I'm racing to this week. Aiming to wrap them up this weekend when I get to put a few more hours into it.

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

  • Write unit tests for cmdline API functions
  • Add support for :repl-fn option (see repl-y link referenced above)
  • Add relevant unit tests for repl-fn option
  • Run lein test-all
  • Run lein with-profile cljfmt cljfmt check
  • Run lein with-profile eastwood eastwood
  • Run lein docs
  • Squash commits
  • Rebase
@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

Should the :repl option be used to replace a call to cmdline.-run-repl?

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

Where exactly?

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

I’m thinking of putting the conditional testing logic for a repl function in the interactive-repl function on line
https://github.com/nrepl/nrepl/pull/116/files#diff-157defd03be20ee96b1f635d628a73a3R385. Then moving it up to line
https://github.com/nrepl/nrepl/pull/116/files#diff-157defd03be20ee96b1f635d628a73a3R334

So it can be reused by the connect-to-server function.

Then create a options->repl-fn that uses require-and-resolve to look for a repl-fn cli arg or :repl-options property following the patterns in the code for resolving options that already exists :)

@bbatsov bbatsov referenced this pull request Jan 23, 2019

Merged

New print middleware #118

@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Yeah, that seems reasonable.

@eccentric-j eccentric-j force-pushed the eccentric-j:refactor-cmdline branch from 2b82895 to 318525c Jan 30, 2019

eccentric-j added a commit to eccentric-j/nrepl that referenced this pull request Jan 30, 2019

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

  • Write unit tests for cmdline API functions
  • Add support for :repl-fn option (see repl-y link referenced above)
  • Add relevant unit tests for repl-fn option
  • Run lein test-all
  • Run lein with-profile cljfmt cljfmt check
  • Run lein with-profile eastwood eastwood
  • Run lein docs
  • Squash commits
  • Rebase

@eccentric-j eccentric-j force-pushed the eccentric-j:refactor-cmdline branch from 318525c to 57ccc62 Jan 30, 2019

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

I completed the outstanding tasks. Unfortunately the CI tests failed, which appears to be due to the printing changes. I'm unsure if the cmdline changes are affecting the tests, please let me know if you suspect some side effect I should be trying to track down.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 31, 2019

Codecov Report

Merging #116 into master will increase coverage by 2.88%.
The diff coverage is 58.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   77.19%   80.07%   +2.88%     
==========================================
  Files          15       15              
  Lines        1289     1350      +61     
  Branches       49       59      +10     
==========================================
+ Hits          995     1081      +86     
+ Misses        245      210      -35     
- Partials       49       59      +10
Impacted Files Coverage Δ
src/clojure/nrepl/cmdline.clj 51.69% <58.55%> (+30.55%) ⬆️
src/clojure/nrepl/transport.clj 60.18% <0%> (+0.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 754a73c...57ccc62. Read the comment docs.

@bbatsov bbatsov merged commit 176bde8 into nrepl:master Feb 4, 2019

2 of 3 checks passed

codecov/patch 58.55% of diff hit (target 77.19%)
Details
codecov/project 80.07% (+2.88%) compared to 754a73c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

The failure is in the code coverage check, which is notoriously buggy anyways. I’ll merge the PR in it’s current form and down the road I’ll try to find a bit of time to tweak the API and its tests further.

Thanks for tackling this!

@eccentric-j

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Thanks so much for your help! I do have some availability if there's anything that I can do to help improve the API or tests for it, perhaps assign an issue to me if a related one comes up? I enjoyed collaborating on this with you and the team.

bbatsov referenced this pull request Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.