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

Change representation of debug-commands to a set of command names #654

Merged
merged 2 commits into from
Oct 19, 2019

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Oct 16, 2019

Note: This is a breaking change that sends debug commands to the client as an unsorted
list of command names without key mapping information, as a means of separating
concerns.

Quick draft of the changes suggested in clojure-emacs/cider#2731
If everything looks alright I'll go ahead with adding changelog notes and tests

Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

"t" :trace})
"An unsorted set of commands supported by the debugger."
#{:continue
:Continue
Copy link
Member

Choose a reason for hiding this comment

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

We should use a proper name here - the current one was just to convey the relationship with C.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:continue-all and :inspect-prompt? Or maybe :inspect-current for p
Would that also call for a command name -> display name mapping on the client side? Right now it's just using the plain command names as overlay titles.

Copy link
Member

Choose a reason for hiding this comment

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

You read my mind. I was just writing about some display name mapping on the other ticket. :-)

:continue-all

👍

For inspect - let's have the default command inspect the current value and the other to be inspect-prompt.

:next
:out
:inspect
:insPect
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@bbatsov
Copy link
Member

bbatsov commented Oct 16, 2019

Btw, I guess the second inspect should go in a different PR, as we don't really have that command available currently.

This is a breaking change that sends debug commands to the client as an unsorted
list of command names without key mapping information, as a means of separating
concerns.
@yuhan0
Copy link
Contributor Author

yuhan0 commented Oct 17, 2019

Ok, I rebased the conflicts, removed the references to :insPect and renamed :Continue to :continue-all.

Somehow I can't seem to get the test suite working locally - make test reports an unrelated error in the info-test and passes the cider.nrepl.debug tests.

FAIL in (integration-test) (info_test.clj:161)
info op get info of a java method
expected: (.startsWith (:javadoc response) "https://docs.oracle.com/")
  actual: false

@bbatsov
Copy link
Member

bbatsov commented Oct 17, 2019

@yuhan0 I see the debug integration test is failing currently on the CI. Seems like a real failure to me.

@yuhan0
Copy link
Contributor Author

yuhan0 commented Oct 19, 2019

Ok it was a fairly simple change, now all existing tests are passing :)

@bbatsov
Copy link
Member

bbatsov commented Oct 19, 2019

@yuhan0 Just squash the last two commits and we're good to go.

Update :Continue -> :continue-all in debug-integration-test
@bbatsov bbatsov merged commit 3762b9e into clojure-emacs:master Oct 19, 2019
@bbatsov
Copy link
Member

bbatsov commented Oct 19, 2019

Great work! Thanks for tackling this! 🙇

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.

2 participants