cljr-slash using suggest-libspecs middleware op
#532
Conversation
Calls the cljr middleware `suggest-libspec`, specifying a string representing the alias-ref, a tuple specifying language context, and capturing the `cljr-magic-require-namespaces` defcustom.
Detects the context of buffer and at point, returning a tuple consisting of the language context of the buffer, and the language context at the current point.
If the `cljr-slash-uses-suggest-libspec` defcustom is enabled, the libspec recommendations will come from the `suggest-libspec` middleware instead of `namespace-aliases` by way of the `cljr--prompt-or-select-libspec` method. No longer filtering the resulting libspec to check if cljr--in-namespace-declaration is true, as that should be handled by the backend, or if the user types in their own completing-read value, presumably they want to include that regardless of the existing namespaces. Completing-read is allowed to return user input, regardless of whether that is a known libspec.
There was a problem hiding this comment.
This looks great to me. Thanks much!
I assume you've tried this over the real middleware we released for refactor-nrepl.
- Does it work correctly?
- Do you need this PR merged asap, or are you comfortable by working with your clj-refactor fork day-to-day?
- Do you have any new feedback item for the refactor-nrepl middleware, or shall we implement it as planned?
Cheers - V
clj-refactor.el
Outdated
| Passes through the custom `cljr-magic-require-namespaces' so that | ||
| users can specify default recommended alias prefixes that may not | ||
| appear in the project yet." | ||
| (seq-let (buffer-context point-context) language-context |
There was a problem hiding this comment.
seq-let is probably nice but this lib (and I, coincidentally) only use seq.el for "seq stuff", not for other conveniences which would increase cognitive load to casual maintainers.
I'm generally happy with let* (and prefer let* over let, no matter what)
There was a problem hiding this comment.
I've addressed this to your recommendation. From my perspective, destructuring binding is much easier to follow over manually parsing out car/cadr of each element, but to each their own. I appreciate your concern about cognitive load from unfamiliar constructs, but it's also frustrating and discouraging as a contributor to so frequently encounter code included in emacs core that is then dismissed at review. Do you have a style guide somewhere that I could follow to discover which are recommended and which are discouraged instead of discovering all of these at review time?
There was a problem hiding this comment.
I could follow to discover which are recommended and which are discouraged instead of discovering all of these at review time?
Not that I know of
Generally, afaict in clojure-emacs .el code we lean toward simple constructs. Even then, different reviewers might have different preferences.
My genuine recommendation would be to simply be ready to tweak some things upon request. I myself cannot predict the feedback I'll receive in the variety of repos I contibute to - some little churn on superficial things is just part of life! :)
Uncovers an existing infinite loop in one of the methods used to parse reader conditionals.
Co-authored-by: vemv <[email protected]>
… `cljr--language-context-at-point`
…nt`. See clojure-emacs#533 for explanation of what needs to happen to re-enable.
|
I believe I've addressed all outstanding concerns, updated external documentation and added a changelog entry as well as manually retesting it in a project. I would much prefer we merge this over letting it bitrot in review. It's not perfect but it will allow folks to start testing and working off of the new middleware so we can iterate from there. I believe the next steps beyond this PR on the cljr-refactor side are to address the reader conditional parsing in #533, and then document the new syntax for I believe that API is solid enough to move forward with. I didn't encounter anything critical with it in development, but now that the logic has moved into the middleware, it's likely we won't see issues in the recommendations until we encounter them in actual use. Let me know if there is any other critical concerns that need to be addressed prior to merge. |
|
Just checking back in to see what else needs to be done here before it's merged. |
|
Thanks for the ping! Will check out today / tomorrow at most |
|
Thanks again for this first step! Should clojure-emacs/refactor-nrepl#384 be implemented as planned? Anything that came to mind more recently? |
|
I think we can proceed with the remainder of clojure-emacs/refactor-nrepl#384 as planned. However, until #533 is implemented, we won't know the correct current language context at point. My expectation remains that we will have more tweaking required on the exact recommendations from the backend, but that won't be as visible until it's in use. I'll see if I can take a stab at #533 in the coming week or two, though so it can operate with the correct local information. Thanks for merging this, and the review! |
|
Nice! I'll try to advance on the middleware, do feel free to ping if you'd need to have it sooner. |
Implements #531.
Still need to add changelog/readme updates if this approach is accepted.If the defcustom
cljr-slash-uses-suggest-libspecis enabled,cljr-slashwill query the newsuggest-libspecmiddleware op for suggested namespaces.Currently it sends over the entire
cljr-magic-require-namespacesas the preferred-aliases map, that may require pre-processing at some point, but the placeholder seems correct. It also correctly identifies if code block is in a conditional language context, and provides that information to the middleware to in order to assist generating recommendations.Finally, as it's using
completing-readto select namespaces, if the user inserts their own content into the response, that will be inserted as the new libspec. This allows the user to reference a new libspec, or edit it to include refers or require-macro. In the future, it might make sense to use the universal argument to temporarily forcecljr-magic-requiresto use :prompt, allowing a user to insert a require of their own choice more easily.Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)./run-tests.sh)cljr-slash-uses-suggest-libspecdefcustomThanks!