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

added function tap-current-value to inspector #167

Merged
merged 8 commits into from
Mar 6, 2023

Conversation

behrica
Copy link
Contributor

@behrica behrica commented Jan 31, 2023

This PR is a pre-requisite to work on clojure-emacs/cider#3311
It adds a new function to the inspector to tap> the current value.

This function would needed to be called in a new operation in the cider-nrepl middleware.

  • 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 changelog (if adding/changing user-visible functionality)

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for this one.

test/orchard/inspect_test.clj Outdated Show resolved Hide resolved
test/orchard/inspect_test.clj Outdated Show resolved Hide resolved
test/orchard/inspect_test.clj Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@behrica behrica requested a review from vemv February 2, 2023 21:12
@behrica
Copy link
Contributor Author

behrica commented Feb 4, 2023

I have always a certain test failing, but already before my change:

FAIL in (classpath-resources-test) (classpath_test.clj:91)
Iterating classpath resources returns non-empty lists
[#object[java.net.URL 0x17240340 "file:/mnt/extHDD/sources/orchard/src-spec-alpha-2/src/main/clojure/"] ()]
expected: (seq entry)
  actual: (not (seq ()))

@behrica
Copy link
Contributor Author

behrica commented Feb 4, 2023

I have always a certain test failing, but already before my change:

FAIL in (classpath-resources-test) (classpath_test.clj:91)
Iterating classpath resources returns non-empty lists
[#object[java.net.URL 0x17240340 "file:/mnt/extHDD/sources/orchard/src-spec-alpha-2/src/main/clojure/"] ()]
expected: (seq entry)
  actual: (not (seq ()))

Got it, need to test via make test

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

I will add it, but I think we test the clojure.core/tap> function doing that, not the code of cider / orchard.

It can also be understood as "we're tapping the right value" i.e. the integration with tap is correct.

Will be merging+releasing after one last touch. Great work!

test/orchard/inspect_test.clj Outdated Show resolved Hide resolved
@behrica behrica requested a review from vemv February 8, 2023 20:14
@behrica
Copy link
Contributor Author

behrica commented Feb 21, 2023

Is there something missing for this PR ?

@vemv
Copy link
Member

vemv commented Feb 21, 2023

Thanks for the ping!

Please fix the linting fault (per the CI checks) and I'll look at it again.

and failing test
@behrica
Copy link
Contributor Author

behrica commented Feb 21, 2023

done

@@ -174,7 +176,7 @@
(defn tap-current-value
"Tap the currently inspected value."
[inspector]
(tap> (:value inspector))
(maybe-tap> (:value inspector))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it a noop on older Clojure

@vemv
Copy link
Member

vemv commented Feb 23, 2023

Hey there!

Please fix the failing tests so that I can merge this.

Cheers - V

@behrica
Copy link
Contributor Author

behrica commented Feb 23, 2023

Done, now tests run all all Clojure versions

@bbatsov bbatsov merged commit 625a78a into clojure-emacs:master Mar 6, 2023
@bbatsov
Copy link
Member

bbatsov commented Mar 6, 2023

Thanks!

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