clojure-emacs / clj-refactor.el

A CIDER extension that provides powerful commands for refactoring Clojure code.
GNU General Public License v3.0
771 stars 111 forks source link

Enable `cljr-slash-uses-suggest-libspec` by default #554

Closed dgtized closed 10 months ago

dgtized commented 10 months ago

Also mark functions and defcustoms that will deprecate once suggest-libspec is the only path available, and expanded the documentation on the feature flag to provide context.

This is a step towards deprecating the old cljr-slash behavior provided by the namespace-aliases middleware started in #531 and implemented in #532. We have had this as an optional feature flag for over a year in total, so now we are trying to transition folks over to make it the default. Presuming no issues, we can then deprecate support for the old middleware.

As #530 is not complete, it's still not possible to specify language context in magic requires. However, even without that functionality, this implementation is a marked improvement over the old two stage prompt for language context, and then prompt for candidate requires. #530 was addressed by https://github.com/clojure-emacs/refactor-nrepl/pull/392.

In the process of looking for future deprecations I also noted that cljr-suggest-namespace-aliases no longer appears to be an option we pass to the middleware. I believe we do this by default, but might be worth double checking before fully deprecating.


Before submitting a PR make sure the following things have been done (and denote this by checking the relevant checkboxes):

Thanks!

vemv commented 10 months ago

As https://github.com/clojure-emacs/clj-refactor.el/issues/530 is not complete

What's missing?

Note that https://github.com/clojure-emacs/refactor-nrepl/blob/7cc8365fa910e74e4c6d2d85f4d3d11f8c7e6618/src/refactor_nrepl/ns/suggest_libspecs.clj#L17 parses the format laid out in https://github.com/clojure-emacs/clj-refactor.el/issues/530#issuecomment-1221624825

Have you given it a try? After that, possibly update docstrings/etc and we'd close #530

In the process of looking for future deprecations I also noted that cljr-suggest-namespace-aliases no longer appears to be an option we pass to the middleware.

The related middleware option is named "suggest" and is correctly passed by the client and handled by the middleware

Cheers - V

dgtized commented 10 months ago

If #530 is done then I guess we can close it or mark it as pending documentation. This has been going on for long enough I've forgotten the context. Is suggest_libspecs_test.clj the tests for that? I think the only reason this change would get blocked by that is if the new format parser is not backward compatible, but I think it allows for both the old and new. I think we can continue to update docs as need be, but I think it's most important we start getting user feedback with the new middleware and then respond to any changes there and continue to iterate on documentation. My assumption is that you and I are the only ones currently using the new middleware which seems very subpar for user testing.

Re cljr-suggest-namespace-aliases, happy to hear that is handled I just noticed it looked dead with the flag enabled.

vemv commented 10 months ago

Is suggest_libspecs_test.clj the tests for that? I

Yes

, but I think it allows for both the old and new.

Yes, definitely

If https://github.com/clojure-emacs/clj-refactor.el/issues/530 is done then I guess we can close it or mark it as pending documentation. This has been going on for long enough I've forgotten the context.

I'd appreciate if at least one person other than me verified this to work. Also, our normal standard is to document first, release second.

If you absolutely didn't have the time I'd understand. I'll leave the PR open for a few days to leave a reasonable headroom.

Cheers - V

dgtized commented 10 months ago

We can keep improving the docs, there are a lot of shortfalls for both refactor-nrepl and clj-refactor.el, and I don't think this particular PR should be responsible for filling that back in. We know the suggest middleware is handling the current input format correctly so it won't break for existing users when we enable the flag, and then we can start moving folks over to other new features as we document them and do more verification then.

The reason I had forgotten that #530 was completed is there was no link between https://github.com/clojure-emacs/refactor-nrepl/pull/392 to #530 when it was done, and then there was no corresponding update to the documentation here or the tests. I don't think that was assigned to anyone. For that matter, only namespace-aliases is documented there but the documentation for suggest appears to be missing entirely? I don't think that was released prematurely without documentation, but it would be beneficial to add it there.

I believe https://github.com/clojure-emacs/clj-refactor.el/wiki#magic-requires is the only non-code documentation of this feature from the clj-refactor.el side. It doesn't reference the new magic requires syntax, but is otherwise mostly accurate. I can update the reference to the feature flag once this PR has merged. In the future, I can also take a look at referencing or summarizing updated docs for the suggest middleware in refactor-nrepl from clj-refactor.el side or start incrementally distilling our various discussions on all of these PRs and issues. However, this is the time and scope I have to contribute for this week, baring any technical concerns of this PR that need fixing.

Follow ups that this feature still need in the long term, so either of us can contribute when we have time:

I'm trying to incrementally move this forward, so it does not languish. I appreciate that fully documenting this feature is valuable, but I think getting it in the hands of users is most important. My hope is that by enabling this feature it will also help triage friction points that most need documentation.

vemv commented 10 months ago

Thanks much for laying out what's next.

I'll merge, and cut a stable+announced release a few days later as I have the chance to carefully consider our observations.

dgtized commented 9 months ago

I updated the wiki for https://github.com/clojure-emacs/clj-refactor.el/wiki#magic-requires to focus more on cljr-slash and the capabilities the default suggest-libspec middleware provide. It still needs further details on the custom syntax for cljr-magic-require-namespaces but now it's at least mentioning language contexts.

vemv commented 9 months ago

Thank you!