Skip to content

Conversation

Malabarba
Copy link
Member

cider-debug-locals

@Malabarba
Copy link
Member Author

It would be nice to display functions in a prettier way, but I guess that's more of a low-level cider thing, not really a role for the debugger.

cider-debug.el Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think when the user sets this to t in a defcustom they expect the locals to always be shown and that any use of l to toggle locals be scoped to the current debugging session.

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, but that was a lot more work (we just don't have a notion of debugging session yet =/), so I compromised a bit and left the toggle as persistent.

I could make it so that l toggles locals only for this debugging step. That would be very simple, but I don't know if that's preferable to the current state.

Copy link
Member

Choose a reason for hiding this comment

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

How about just renaming it to cider--debug-display-locals and making it a defvar instead of a defcustom?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Will do.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure making this a defvar would be a step in the right direction. A private defvar is not something you see every day.

Copy link
Member

Choose a reason for hiding this comment

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

I had in mind inspecting them.

Anyways, getting everything right the first time around is not super important. We just have to have a sane version of this feature to merge.

Generally, even the current state is acceptable to me as I don't share @expez's expectations about how the features would work - I'd expect it to work exactly as it does (although it'd be nice to have something affecting the scope of the session that doesn't exist).

Copy link
Member

Choose a reason for hiding this comment

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

[...] even the current state is acceptable to me

I really think we should get rid of the defcustom, it doesn't represent anything a user should toggle in its current state.

Copy link
Member

Choose a reason for hiding this comment

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

Well, you'll either have locals all the time or you won't. I'm fine with not having them by default and being able to globally toggle them, I'm also fine with being able to toggle them just for the current expression. Having this feature is an improvement, we can always refine it down the road (Lean for the win).

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no strong opinions on the variable. As long as the user doesn't have to hit l at every step this is fine by me. I'll remove it when I sit down for this again.

As for the notion of debugging session. I've thought of a way to implement this at the clojure side that wouldn't be too complicated, I just never had too much of a reason to (this is the first feature that misses it). But I'd rather keep things simple for 0.9, I already short on time as it is.

Copy link
Member

Choose a reason for hiding this comment

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

OK, let's just remove the variable and merge this.

@Malabarba
Copy link
Member Author

Pushed.

bbatsov added a commit that referenced this pull request Jun 12, 2015
[Fix #1106] Display local variables in the debugger
@bbatsov bbatsov merged commit 3487955 into clojure-emacs:master Jun 12, 2015
@bbatsov
Copy link
Member

bbatsov commented Jun 12, 2015

👍 Looks good!

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