Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Reorder repl init to occur after debugger init #1950

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

dpsutton
Copy link
Contributor

@dpsutton dpsutton commented Mar 5, 2017

When initializing the repl, we ask what namespace we are in by evaling code,
eg (eval "(str ns)"). However, enlighten mode hijacks eval messages in the
middleware and uses the debugger on them which causes an error if the debugger
is not yet initialized.

#1947

  • The commits are consistent with our [contribution guidelines][1]
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (make test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)
  • You've updated the refcard (if you made changes to the commands listed there)

@bbatsov
Copy link
Member

bbatsov commented Mar 5, 2017

LGTM, the relevant issue has to be mentioned in the commit message [Fix ... and the changelog.

cider.el Outdated
(cider--check-required-nrepl-version)
(cider--check-clojure-version-supported)
(cider--check-middleware-compatibility)
(cider--debug-init-connection)
;; cider-repl-init evals code. if enlighten mode is on, eval requires the
Copy link
Member

Choose a reason for hiding this comment

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

`cider-repl-init'

if `enlighten-mode' is on

@bbatsov
Copy link
Member

bbatsov commented Mar 5, 2017

In the interest of robustness we might also add some check in the enlighten init code to determine whether it can be enabled properly or not. That's not essential for sure, but would be nice to have.

@dpsutton dpsutton force-pushed the feature/reorder-jack-in branch 2 times, most recently from 8ce7112 to ef47c0f Compare March 5, 2017 15:29
@bbatsov
Copy link
Member

bbatsov commented Mar 5, 2017

Don't forget about the changelog entry.

@dpsutton dpsutton force-pushed the feature/reorder-jack-in branch from ef47c0f to 264eaa3 Compare March 5, 2017 15:42
@dpsutton
Copy link
Contributor Author

dpsutton commented Mar 5, 2017

Ah so sorry. Got that updated.

In regards to your thoughts about checking that enlighten mode can actually run yet, did you want that to be on the elisp side or the clojure side?

@bbatsov
Copy link
Member

bbatsov commented Mar 5, 2017

In regards to your thoughts about checking that enlighten mode can actually run yet, did you want that to be on the elisp side or the clojure side?

Elisp.

@dpsutton
Copy link
Contributor Author

dpsutton commented Mar 7, 2017

So I put the inhibition in and "undid" the reorder. Read the commit message and see if you agree. If so, I think we are ready to squash and merge.

Also, in the related issue, I thought this was going to be much worse. I was thinking that the startup tasks were spread throughout the codebase, but that was just the two session creation points (tooling and regular). That made me think all the initialization happened at random points. I was happy to be misinformed on that point.

@bbatsov
Copy link
Member

bbatsov commented Mar 7, 2017

👍 I love the new approach. No idea why this wasn't my initial suggestion, as it's clearly the natural way to solve this problem.

…ization

When initializing the repl, we ask what namespace we are in by evaling code,
eg (eval "(str *ns*)"). However, enlighten mode hijacks eval messages in the
middleware and uses the debugger which causes an error if the debugger is not
yet initialized, ie, in `cider-repl-init`.

So we inhibit this feature. The reason I am doing this let binding rather than
just reordering is that reordering is brittle and doesn't convey that this order
is very important. While its kinda gross that we are dynamically altering this
variable several call stacks up, this is a very special case right at startup
and not complicated logic during the principal use of CIDER.
@dpsutton dpsutton force-pushed the feature/reorder-jack-in branch from 8d251b4 to 7ff6f50 Compare March 8, 2017 14:01
@dpsutton
Copy link
Contributor Author

dpsutton commented Mar 8, 2017

Swuashed and commit message cleaned up a little. I think it's ready to merge.

@bbatsov bbatsov merged commit 27374da into clojure-emacs:master Mar 8, 2017
@bbatsov
Copy link
Member

bbatsov commented Mar 8, 2017

👍

@dpsutton dpsutton deleted the feature/reorder-jack-in branch October 26, 2019 16:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants