Skip to content

Introduce cljr-building-ast-cache? #408

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

Merged
merged 1 commit into from
May 26, 2018
Merged

Introduce cljr-building-ast-cache? #408

merged 1 commit into from
May 26, 2018

Conversation

vemv
Copy link
Member

@vemv vemv commented Jan 29, 2018

Allows one to build helpers to reload code at the right time, avoiding clojure-emacs/refactor-nrepl#134

As this is a simple variable, not sure if should go into changelog/readme?

Cheers - Victor

@benedekfazekas
Copy link
Member

perhaps a before and after hook would be much nicer and more useful too. would be easy to implement the above with them and more.

@benedekfazekas
Copy link
Member

and thanks for investing your time and energy :)

@vemv
Copy link
Member Author

vemv commented Jan 29, 2018

✌️

Note that at least in the project I'm hacking on, analysis happens twice on startup:

image

So, for example, a callback that issued a (init) (or variations: (go), etc. A dev facility to start Reloaded workflow) on AST-index-updated would be called twice, which would be undesirable.

Internally, the user-built helper would have to use internal variables. Which is kind of a roundabout way of doing what my proposed commit does.

I had planned to do this in my emacs.d:

(defun try-init ()
  (if cljr-building-ast-cache?
    (message "Try again in a bit")
    (clojure-eval "(repl/init)")))

i.e. the user may have to try the command a second time, which can be simpler than having callbacks, excess variables, etc

WDYT?

@benedekfazekas
Copy link
Member

I don't think it should happen twice. I can only try to reproduce in the evening tho

@benedekfazekas
Copy link
Member

hooks vs variable: I just think hooks are a more generic way for client code to interact with and also a superset of what you are proposing

clj-refactor.el Outdated
(cljr--call-middleware-async
(cljr--create-msg "warm-ast-cache")
(lambda (res)
(cljr--maybe-rethrow-error res)
(cljr--maybe-nses-in-bad-state res)
(setq cljr-building-ast-cache? nil)
Copy link
Member

Choose a reason for hiding this comment

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

This should be the first thing, or it's going to report t because an error was rethrown, right?

@vemv
Copy link
Member Author

vemv commented Jan 29, 2018

  • Integrated PR feedback from both of you
  • Added entry to the changelog
  • Tested out with a sample functionality build on top of this. Works!
  • The "analysis happens twice" thing was project-specific, due to nrepl connecting twice. Clearly unrelated; sorry for the noise

Finally, there's only one byte-compilation warning:

Warning (bytecomp): Unused lexical argument ‘response’

Dunno how to fix, also hesitant about improving it since you may dislike the empty lambdas (see diff) in the first place.

clj-refactor.el Outdated
:group 'cljr
:type 'function)

(defcustom cljr-after-warming-ast-cache-hook (lambda (response))
Copy link
Member

@expez expez Jan 29, 2018

Choose a reason for hiding this comment

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

Think it's customary to use nil as the default here, and then put the funcall below in a when.

In any event, I'd prefer something a little less elegant over a compile warning about unused locals.

@benedekfazekas
Copy link
Member

@vemv any progress on this? no rush, but as far as i can see there is a only a small step missing (see expez's comment)

@vemv
Copy link
Member Author

vemv commented Feb 7, 2018

Hi, sorry I'm having a busy week! I'll do it asap

@benedekfazekas
Copy link
Member

no rush really. just did not want this to slip through the cracks

@raxod502
Copy link
Contributor

This is somewhat unrelated, but you can fix unused argument warnings by prepending the argument name with an underscore (to signify to the compiler that you intend the variable to be unused).

Allows to reload code at the right time, avoiding clojure-emacs/refactor-nrepl#134
@vemv
Copy link
Member Author

vemv commented May 26, 2018

Feedback addressed, branch rebased against master. Tested out again and the callbacks keep working.

@expez expez merged commit 632be9e into clojure-emacs:master May 26, 2018
@expez
Copy link
Member

expez commented May 26, 2018

Thanks for seeing this through, @vemv! 👍

@vemv vemv deleted the patch-1 branch May 26, 2018 15:26
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.

4 participants