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

Improve inspector #24

Merged
merged 6 commits into from
Mar 21, 2018
Merged

Improve inspector #24

merged 6 commits into from
Mar 21, 2018

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Mar 20, 2018

Two quite major things coming to the inspector, long overdue.

Separate key/value rendering in maps

We had this long time ago, somehow it was lost, now it's time to bring it back. Having to click twice when navigating maps is really annoying. Besides, this behavior was already implemented for metadata - go figure.

As a nice bonus, the path tracking code is now simpler and more robust.

Separate page numbers for each level of nesting

This was a blatant long-standing bug. By having a single value to represent the current page we lose information when we start navigating up and down the data tree. Page index of the parent collection becomes the page of the child collection. This is very confusing. This PR gives each nesting level its own page number by maintaining a stack of them.


The changes should be fully compatible with CIDER and cider-nrepl – nothing to change there besides, maybe, a few integration tests.

@codecov
Copy link

codecov bot commented Mar 20, 2018

Codecov Report

Merging #24 into master will increase coverage by 0.38%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #24      +/-   ##
=========================================
+ Coverage   76.71%   77.1%   +0.38%     
=========================================
  Files          10      10              
  Lines         889     891       +2     
  Branches       40      40              
=========================================
+ Hits          682     687       +5     
+ Misses        167     164       -3     
  Partials       40      40
Impacted Files Coverage Δ
src/orchard/inspect.clj 84.13% <94.11%> (+0.99%) ⬆️

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 ac5efdc...64f55ec. Read the comment docs.

@bbatsov
Copy link
Member

bbatsov commented Mar 21, 2018

Looks great to me! Just two small things:

Some tests on the cider-nrepl side would be appreciated, but are not mandatory.

@alexander-yakushev
Copy link
Member Author

I planned to edit the cider-nrepl part (including tests) once this is merged and an artifact is created. What is the preferred way of co-developing orchard and cider-nrepl?

I will add to the changelog, sure.

@alexander-yakushev
Copy link
Member Author

Regarding #2152, I suffer from this issue too. I was unable to fix it on the Orchard side, but managed to solve it on the cider-nrepl. I will submit that fix together with cider-nrepl tests for this set of changes.

@bbatsov
Copy link
Member

bbatsov commented Mar 21, 2018

I planned to edit the cider-nrepl part (including tests) once this is merged and an artifact is created. What is the preferred way of co-developing orchard and cider-nrepl?

Developing things sequentially is fine.

@bbatsov bbatsov merged commit 4569899 into clojure-emacs:master Mar 21, 2018
@bbatsov
Copy link
Member

bbatsov commented Mar 21, 2018

Merging this PR will immediately deploy a new snapshot to Clojars.

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