Skip to content
This repository was archived by the owner on Jun 2, 2025. It is now read-only.

First pass at macros support #8

Merged
merged 19 commits into from
May 26, 2014
Merged

First pass at macros support #8

merged 19 commits into from
May 26, 2014

Conversation

cichli
Copy link
Member

@cichli cichli commented May 20, 2014

This adds full support for autocompletion and info of macros (both referred and namespace-qualified). Additionally, info now supports referred var names.

There's still a bit of tidying/refactoring to be done - there are a few similar-but-not-quite-the-same functions for macros and vars that could probably be merged, but I'm hesitant to do this until we have support for imports so we can see what that looks like. Additionally, I'm still quite new to Clojure and am not 100% sure of the most appropriate way to go about creating a common abstraction over them - any suggestions for improvement / comments would be much appreciated :).

@gtrak
Copy link
Contributor

gtrak commented May 20, 2014

Any chance you could add some tests? The first changes were much smaller,
but it would be nice if we had tests for those, too.

There's core.async namespaces that you can use for macros, included in the
repo is a compiler dump from a clojurescript project.

On Mon, May 19, 2014 at 8:05 PM, cichli notifications@github.com wrote:

This adds full support for autocompletion and info of macros (both
referred, including cljs.core, and namespace-qualified). Additionally, info
now supports referred var names.

There's still a bit of tidying/refactoring to be done - there are a few
similar-but-not-quite-the-same functions for macros and vars that could
probably be merged, but I'm hesitant to do this until we have support for
imports so we can see what that looks like. Additionally, I'm still quite
new to Clojure and am not 100% sure of the most appropriate way to go about
creating a common abstraction over them - any suggestions for improvement /

comments would be much appreciated :).

You can merge this Pull Request by running

git pull https://github.com/cichli/cljs-tooling master

Or view, comment on, or merge it at:

https://github.com/gtrak/cljs-tooling/pull/8
Commit Summary

  • Add helper fns to analysis.clj for macro support.
  • Add macro support to complete.clj.
  • Tidy complete.clj.
  • Fix referred-vars and referred-macros to conform to doc string.
  • Add support for referred vars to info.clj.
  • Add macro support to info.clj.
  • Tidy analysis.clj.
  • Add cljs.core macros to unscoped completions.
  • Add support for cljs.core macros to info.clj.

File Changes

  • M src/cljs_tooling/complete.cljhttps://github.com/gtrak/cljs-tooling/pull/8/files#diff-0(57)
  • M src/cljs_tooling/info.cljhttps://github.com/gtrak/cljs-tooling/pull/8/files#diff-1(59)
  • M src/cljs_tooling/util/analysis.cljhttps://github.com/gtrak/cljs-tooling/pull/8/files#diff-2(80)

Patch Links:


Reply to this email directly or view it on GitHubhttps://github.com/gtrak/cljs-tooling/pull/8
.

@gtrak
Copy link
Contributor

gtrak commented May 20, 2014

Refactoring can be done once the functionality's there, I'm not too worried
about the shape of this code at the moment, what's most important is
compatibility with cider-nrepl's expectations, and I've also got my eye on
how to use this for cljs-autodoc in the future:

tomfaulhaber/autodoc#16

On Mon, May 19, 2014 at 8:35 PM, Gary Trakhman gary.trakhman@gmail.comwrote:

Any chance you could add some tests? The first changes were much smaller,
but it would be nice if we had tests for those, too.

There's core.async namespaces that you can use for macros, included in the
repo is a compiler dump from a clojurescript project.

On Mon, May 19, 2014 at 8:05 PM, cichli notifications@github.com wrote:

This adds full support for autocompletion and info of macros (both
referred, including cljs.core, and namespace-qualified). Additionally, info
now supports referred var names.

There's still a bit of tidying/refactoring to be done - there are a few
similar-but-not-quite-the-same functions for macros and vars that could
probably be merged, but I'm hesitant to do this until we have support for
imports so we can see what that looks like. Additionally, I'm still quite
new to Clojure and am not 100% sure of the most appropriate way to go about
creating a common abstraction over them - any suggestions for improvement /

comments would be much appreciated :).

You can merge this Pull Request by running

git pull https://github.com/cichli/cljs-tooling master

Or view, comment on, or merge it at:

https://github.com/gtrak/cljs-tooling/pull/8
Commit Summary

  • Add helper fns to analysis.clj for macro support.
  • Add macro support to complete.clj.
  • Tidy complete.clj.
  • Fix referred-vars and referred-macros to conform to doc string.
  • Add support for referred vars to info.clj.
  • Add macro support to info.clj.
  • Tidy analysis.clj.
  • Add cljs.core macros to unscoped completions.
  • Add support for cljs.core macros to info.clj.

File Changes

  • M src/cljs_tooling/complete.cljhttps://github.com/gtrak/cljs-tooling/pull/8/files#diff-0(57)
  • M src/cljs_tooling/info.cljhttps://github.com/gtrak/cljs-tooling/pull/8/files#diff-1(59)
  • M src/cljs_tooling/util/analysis.cljhttps://github.com/gtrak/cljs-tooling/pull/8/files#diff-2(80)

Patch Links:


Reply to this email directly or view it on GitHubhttps://github.com/gtrak/cljs-tooling/pull/8
.

@cichli
Copy link
Member Author

cichli commented May 20, 2014

Thanks for the feedback - I'll add tests for all the new features this evening. From a quick look at the compiler dump we can cover everything, but it might be useful to generate a new dump with some more complex ns declarations to make sure all the edge cases are caught correctly.

Is there any reason we can't include a cljs project in test-resources, and then analyse that at test time to create a test env? That would make it much easier to add new test cases etc. as issues arise. This will obviously introduce a test-time dependency on clojurescript, but brings the benefit of being able to test against multiple clojurescript versions. Thoughts?

@gtrak
Copy link
Contributor

gtrak commented May 20, 2014

Ah, that could work, too. Just more work, and the compiler dump was an end
in itself at the time. I have a languishing JIRA patch for it.

I don't think there's been analyzer format changes since I started on this,
and the cider-nrepl stuff pretty much won't work on anything older than
2202. In the future (probably a year from now), they're going to integrate
tools.analyzer.js, and I'd like to be able to support it out the gate.

On Tue, May 20, 2014 at 7:05 AM, cichli notifications@github.com wrote:

Thanks for the feedback - I'll add tests for all the new features this
evening. From a quick look at the compiler dump we can cover everything,
but it might be useful to generate a new dump with some more complex ns
declarations to make sure all the edge cases are caught correctly.

Is there any reason we can't include a cljs project in test-resources, and
then analyse that at test time to create a test env? That would make it
much easier to add new test cases etc. as issues arise. This will obviously
introduce a test-time dependency on clojurescript, but brings the benefit
of being able to test against multiple clojurescript versions. Thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/gtrak/cljs-tooling/pull/8#issuecomment-43612102
.

@cichli
Copy link
Member Author

cichli commented May 21, 2014

Yes, adding tests was indeed useful - I've ironed out a bunch of edge cases. The info tests could do with cleaning up a bit and I also couldn't find any cases of :excludes that didn't have locally-declared vars shadowing them anyway, so the completion test for that is commented out for now. I'll take a look at both tomorrow.

One thing to note: I had to add dependencies for clojurescript, core.async and om to be able to load the macros for the new tests. I'm not sure if that's desirable - they're in the :dev profile anyway.

@cichli
Copy link
Member Author

cichli commented May 21, 2014

Oh, and please let me know if you want me to squash the commits.

@gtrak
Copy link
Contributor

gtrak commented May 22, 2014

I'll review this within the next few days, but sounds good so far.

@gtrak gtrak merged commit 5c3191b into clojure-emacs:master May 26, 2014
@gtrak
Copy link
Contributor

gtrak commented May 26, 2014

👏

@cichli
Copy link
Member Author

cichli commented May 26, 2014

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants