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

Send the complete ns form in interactive eval requests #1027

Closed
wants to merge 1 commit into from

Conversation

cichli
Copy link
Member

@cichli cichli commented Mar 17, 2015

Fixes #1026.

See CLJS-1139 - ns forms are not currently additive in ClojureScript REPLs, so until that's fixed, I think we should send the full form.

This was previously changed to send the full form in #807 and then changed back in #814 - I can confirm that the latter has not regressed from this change.

AFAIK the only possible side-effect from this would be the:reload / :reload-all options, which we already remove.

cc @vspinu - does this sound reasonable?

@bbatsov
Copy link
Member

bbatsov commented Mar 18, 2015

See CLJS-1139 - ns forms are not currently additive in ClojureScript REPLs, so until that's fixed, I think we should send the full form.

Yes.

This was previously changed to send the full form in #807 and then changed back in #814 - I can confirm that the latter has not regressed from this change.

Yeah, we were incorrectly believing the ns forms weren't additive; turned out we had another problem.

While we're on this, you might also comment the logic in the function a bit. If it's confusing to you I can imagine few people would normally understand what are we doing here. :-)

@vspinu
Copy link
Contributor

vspinu commented Mar 18, 2015

Oh. We have been dancing around this for at least half a year now. #830 and #956 are behind the most recent code change.

@cchili Could you please just comment those two lines and reference all the relevant issues in the comment? Thanks.

@cichli
Copy link
Member Author

cichli commented Mar 18, 2015

Yeah, I'll add some notes to the code now. FWIW I see little-to-no performance difference between cider-eval-last-sexp and cider-eval-defun-at-point (the former using plain eval and the latter using load-file with the full ns declaration), so it seems performance is no longer an issue. Certainly neither is taking 1-2 seconds even for a big ns declaration.

@cichli
Copy link
Member Author

cichli commented Mar 19, 2015

Hang on, don't merge this yet...

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.

ns-form sent within cider-eval-defun-at-point seems incomplete
3 participants