Fix info lookups from namespaces that don't yet exist #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vemv
reviewed
Sep 7, 2021
src/orchard/info.clj
Outdated
| (some-> (find-ns unqualified-sym) (m/ns-meta))) | ||
| ;; Lookups in files whose namespaces don't exist yet should still be able | ||
| ;; to resolve built-ins and fully-qualified syms | ||
| (recur (assoc opts :ns 'clojure.core))))) |
There was a problem hiding this comment.
Very minor, but the large distance between the if and this branch makes it a bit hard to track the control flow.
By using an if-not instead one gets these "degenerate cases" out of one's way, similarly to https://wiki.c2.com/?GuardClause
There was a problem hiding this comment.
Thanks for the feedback. I've addressed this one. I'll still be adding a test and changelog etc.
plexus
force-pushed
the
lookup-in-undefined-ns
branch
2 times, most recently
from
666599d to
10aee3d
Compare
Currently when doing a `var-info` from a file before evaluating the `ns` form always fails. The namespace object does not yet exist, and so none of the var resolution strategies work. However fully-qualified vars, namespace names, or built-ins (clojure.core functions) could all be resolved, and it's frustrating that they aren't. This adds a check to see if `(find-ns ns)` returns anything. If not then we are initiating the lookup from a namespace that has not yet been defined, in which case none of the lookup strategies will proceed, so we continue with doing the lookup as if it had been initiated from `clojure.core`.
plexus
force-pushed
the
lookup-in-undefined-ns
branch
from
10aee3d to
99875ac
Compare
|
Added a test, appended the CHANGELOG, and swapped the if branches around as suggested by @vemv . I think from my side this is good to go. |
|
Nice improvement. Thanks! |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently when doing a
var-infofrom a file before evaluating thensformalways fails. The namespace object does not yet exist, and so none of the var
resolution strategies work.
However fully-qualified vars, namespace names, or built-ins (clojure.core
functions) could all be resolved, and it's frustrating that they aren't.
This adds a check to see if
(find-ns ns)returns anything. If not then we areinitiating the lookup from a namespace that has not yet been defined, in which
case none of the lookup strategies will proceed, so we continue with doing the
lookup as if it had been initiated from
clojure.core.Before submitting a PR make sure the following things have been done:
Thanks!