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

Fix problematic double var lookup in xref/fn-refs+fn-deps #136

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Oct 11, 2021

fn-deps already calls as-val which will resolve a var if it is given one, so
fn-refs does not need to call (map var-get) beforehand. Not doing this
presents issues when a var contains a symbol. This currently would result in the
symbol being treated as a var name, causing exceptions or incorrect results.

Closes #135

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

Thanks!

@vemv
Copy link
Member

vemv commented Oct 11, 2021

Excellent opportunity to increase test coverage 🙏 sometimes I'm finding our test suites give me bit of a false confidence

@plexus
Copy link
Contributor Author

plexus commented Oct 11, 2021

Be my guest!

@vemv
Copy link
Member

vemv commented Oct 11, 2021

Doesn't bother me 👍

Relatedly perhaps you can open PRs directly from a branch (vs. a fork)? That way one can push directly (which obviously would be only done when it's called for)

For next time, here I believe I'll post a .patch

@vemv vemv marked this pull request as draft October 11, 2021 09:18
@plexus
Copy link
Contributor Author

plexus commented Oct 11, 2021

Oh yeah sure, wasn't aware of the etiquette around that. Note that I have Allow edits by maintainers checked so you should be able to push to my branch I think.

@bbatsov
Copy link
Member

bbatsov commented Oct 18, 2021

@vemv What's the ETA for the tests? I'd like to wrap this up this week if possible.

@vemv
Copy link
Member

vemv commented Oct 18, 2021

Noted, will give it a shot soon

@vemv vemv force-pushed the fix-fn-refs-double-var-lookup branch from 6871c27 to a61ca53 Compare October 18, 2021 10:41
@vemv vemv marked this pull request as ready for review October 18, 2021 10:41
`fn-deps` already calls `as-val` which will resolve a var if it is given one, so
`fn-refs` does not need to call `(map var-get)` beforehand. Not doing this
presents issues when a var contains a symbol. This currently would result in the
symbol being treated as a var name, causing exceptions or incorrect results.
@vemv vemv force-pushed the fix-fn-refs-double-var-lookup branch from a61ca53 to c4f26eb Compare October 18, 2021 10:42
@vemv vemv requested a review from bbatsov October 18, 2021 10:45
@bbatsov bbatsov merged commit 826c2e1 into clojure-emacs:master Oct 18, 2021
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.

cider-xref-fn-refs "invalid namespace" error
3 participants