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

Port cljs info and consolidate info api #37

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@arichiardi
Copy link

arichiardi commented Dec 30, 2018

In order to start consolidating Clojure and ClojureScript tooling under our apple orchard, this patch starts porting the info op from cljs-tooling. Consequently, it paves the way to port some of the orchard Clojure-only code to ClojureScript.

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch 3 times, most recently from 9e91aeb to 89bda38 Dec 30, 2018

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 12, 2019

@bbatsov in porting I noticed that the namespace in Clojure is returned as a Namespace object instead of a symbol...I think we should normalize it and because ClojureScript does not reify namespaces the only option is to use symbols.

Now...I guess it would be breaking at some point up the cider-nrepl chain but I will try to test and document it.

What do you think?

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Jan 12, 2019

I guess that won't be a big deal, as most people use only the nREPL interface exposed by cider-nrepl.

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch 2 times, most recently from 5ba7947 to cf7853d Jan 19, 2019

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 21, 2019

@bbatsov so I ported it finally. This means that after I fix the CI, this is good to go. However, the API changed (will write how in the README) so I think this should go in together with changes is cider-nrepl.

Another note is that this PR does not change behaviors, for instance I have not included specs in the Cljs meta at the moment. That was my original plan and so it might be actually a good idea to review this but keep adding commits to this branch, test everything and then bump all the thing!

What do you think?

@cichli

This comment has been minimized.

Copy link
Member

cichli commented Jan 21, 2019

IMHO it is better to keep the CLJS support as a separate library.

  • Some of the differences between Clojure and ClojureScript are major, some are subtle, and trying to accommodate all of these differences in one place can get painful.
  • Clojure and ClojureScript evolve at different rates. By separating the libs we follow these rates of change separately.
  • The self-host stuff is just not relevant to the CLJ parts of this library (N.B. I have simplified the self-host build and CI upstream, so if we do merge this, I would appreciate those changes being merged too).
  • With this PR the git history of the existing library is lost. This is valuable information when debugging/changing the code, and it's a shame to lose it unnecessarily.

I think it's more reasonable to just reimplement the spec stuff in cljs-tooling. We could make a shared library for any truly common code, but we should be careful about this, because if at some point in the future either CLJ or CLJS diverges in behaviour from what the shared library expects, we get in a mess.

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 21, 2019

IMHO it is better to keep the CLJS support as a separate library.

I thought about this, but there are two major disadvantages:

  • Cljs tooling is never growing with the Clj tooling (yes for me this is a problem because I use Cljs 😄)
  • Nobody is willing to maintain yet another library

Note that the behavior is still separate in the code, the :dialect key controls which function will be called, but now we have better testing for info. Also, things like cleaning up and normalization of the generated return map now can be extracted and I really saw in the code that there are many places where there are too many things going on at the same time and should be refactored.

Clojure and ClojureScript evolve at different rates. By separating the libs we follow these rates of change separately.

Semantically they are the same language so they should have the same API for retrieving info and even completion I maintain. You mention it as a cons of porting, I would mention it as a pro. Having said that, things are still separate.

The self-host stuff is just not relevant to the CLJ parts of this library (N.B. I have simplified the self-host build and CI upstream, so if we do merge this, I would appreciate those changes being merged too).

I don't have a clear vision here of why self-host is not relevant, given we have conditional readers, which is a Clojure feature, I don't see why self-host should not be considered. In particular, for many of the info stuff returned there could be a self-host server that actually returns it. Having said that, I think in this first iteration I might just get the Cljs part in, I don't know yet, depends on how much time I have.

With this PR the git history of the existing library is lost. This is valuable information when debugging/changing the code, and it's a shame to lose it unnecessarily.

What do you mean by this? This is a PR and as every commit you have git history, am I missing something?

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 21, 2019

Oh I see you where talking about the .clj to .cljc rename. I could do it in stages for sure. It would be probably cleaner. So no self-host for now. How does it sound?

@cichli

This comment has been minimized.

Copy link
Member

cichli commented Jan 21, 2019

Nobody is willing to maintain yet another library

I am willing to do that.

Also, things like cleaning up and normalization of the generated return map now can be extracted and I really saw in the code that there are many places where there are too many things going on at the same time.

This sounds like a good change but it's not clear what you mean looking at the diff in Github. I will look locally with --find-renames later.

Semantically they are the same language so they should have the same API for retrieving info and even completion I maintain.

They are different in quite a few ways. We can still maintain equivalent APIs in two libraries, right?

I don't have a clear vision here of why self-host is not relevant

For example, the classpath and javadoc features.

What do you mean by this? This is a PR and as every commit you have git history, am I missing something?

Your PR doesn't contain the commits from the other library in e.g. the analysis namespace. Tools like git blame and git bisect don't work as well after code churn like this. AFAIK it is possible to carry the history across to another repo (I believe this was done when some parts of cider-nrepl were originally moved into this library). It's not a dealbreaker but no need to avoid it if we can?

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 21, 2019

They are different in quite a few ways. We can still maintain equivalent APIs in two libraries, right?

Why would you want that and what benefits does it bring? Then every time there is a change in clj you have to replicate it in cljs-tooling and I have already seen that this does not happen. This is also publicly evident from the feature disparity between the two. Your point would be valid but unfortunately syncing things never happens in reality.

For example, the classpath and javadoc features.

I contributed to lumo for quite a while and I feel that Cljs imposes the classpath constraint on any self-host implementation. That is why lumo has a lumo.classpath layer to replicate what the JVM version has.
I don't think javadoc is relevant as a feature at all here because it does not involve meta reading and of course would not appear in a JS runtime

I am willing to do that.

While I really trust that you would maintain the code, there are just so much more benefits in having a central place where all the code goes. For example orchard is used by Calva as well, which now has to include two libraries and do chores, bump, all that. With one solution Orchard would become the only thing you need in order to have feature parity with Cider. This is a dream for now but this is a good start.

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 21, 2019

library in e.g. the analysis namespace. Tools like git blame and git bisect don't work as well after code churn like this. AFAIK it is possible to carry the history across to another repo (I believe this was done when some parts of cider-nrepl were originally moved into this library).

I see this as the weakest of the cons you have made so far so I would really ignore it for this discussion if you agree to do so. In my experience testing things properly has always been more important than git history but maybe I just got lucky 😉

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch from cf7853d to 2bc3d00 Jan 22, 2019

@arichiardi arichiardi changed the title WIP - Port cljs info and consolidate info api Port cljs info and consolidate info api Jan 22, 2019

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch 5 times, most recently from e2e8a4e to 69d85e0 Jan 22, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 22, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@0481244). Click here to learn what that means.
The diff coverage is 84.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #37   +/-   ##
=========================================
  Coverage          ?   83.95%           
=========================================
  Files             ?       16           
  Lines             ?     1340           
  Branches          ?       62           
=========================================
  Hits              ?     1125           
  Misses            ?      153           
  Partials          ?       62
Impacted Files Coverage Δ
src/orchard/namespace.clj 88.73% <ø> (ø)
src/orchard/cljs/special.cljc 100% <100%> (ø)
src/orchard/meta.clj 65.62% <55%> (ø)
src/orchard/cljs/meta.cljc 83.05% <83.05%> (ø)
src/orchard/cljs/analysis.cljc 83.87% <83.87%> (ø)
src/orchard/misc.clj 74.07% <88.46%> (ø)
src/orchard/classpath.clj 89.65% <88.88%> (ø)
src/orchard/info.clj 91.66% <92.4%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0481244...a4ee30e. Read the comment docs.

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Jan 22, 2019

@bbatsov this now is all green and does not include self-host and clj->cljc renaming in src. Just some in the tests.

Ready for review.

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch from 69d85e0 to 799e629 Jan 22, 2019

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Jan 23, 2019

I’m on a vacation with a computer for a while. I’ll leave the review part to @cichli and @SevereOverfl0w in my absence. :-)

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Feb 8, 2019

@cichli and @SevereOverfl0w and @bbatsov

My next task, and I would like to start today maybe, would be to copy the leftover "business logic" from the info middleware to the orchard info function. This will make the info function in orchard self-contained, which is great, easier to test and more portable (one could use orchard without the nrepl protocol for instance - say for a Language Server Protocol implementation).

Having said that, it's been a couple of weeks and I did not see any further activity on this PR, so I am going to ask the very controversial question "is anybody interested in seeing this in?". I know for instance @cichli is against it, but I hoped I could convince him otherwise 😄

If not, well I will just working end on it or fork it 👍

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Feb 11, 2019

I won’t be around until early March. In the mean time you’ll have to figure out ways to make progress without me.

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch from 166a87f to 4a53f5a Feb 15, 2019

Show resolved Hide resolved project.clj Outdated
@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 29, 2019

Why did you change the format of the see-also.edn exactly? The current format is exactly what we get from an export from clojuredocs.org. Changing it would also require writing some task to pull and transform the data from there.

Show resolved Hide resolved project.clj
@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Mar 29, 2019

Why did you change the format of the see-also.edn exactly? The current format is exactly what we get from an export from clojuredocs.org. Changing it would also require writing some task to pull and transform the data from there.

Oh well didn't exactly know that, it is not written anywhere 😄

It looked like we were doing extra work for the conversion to string->symbol and viceversa all the time. I will bring it back as it was.

@@ -51,3 +53,21 @@
(classpath-jarfiles (classpath)))
([path]
(map #(JarFile. ^File %) (filter cp/jar-file? path))))

(defn classpath-file-relative-path

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 29, 2019

Member

Don't we have a patch about this in the classpath function already?

This comment has been minimized.

Copy link
@arichiardi

arichiardi Mar 29, 2019

Author

I will double check but this was in the info middleware in cider-nrepl and it's been ported over here

This comment has been minimized.

Copy link
@arichiardi

arichiardi Apr 4, 2019

Author

Not sure about what is going on in this function actually, so I would rather leave it here for now cause it can break functionality to change it. I can open a separate issue when merged to track it.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 29, 2019

Not strictly necessary in this PR but it would be really nice if we could remove any dependencies on core.async, mount, etc. from the CLJS tests. In hindsight, the convenience this offers isn't worth how painful it makes testing multiple CLJS versions. I think any examples we need for testing, we should define ourselves.

👍 The simpler the setup is, the better.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 29, 2019

The README should also be updated to reflect to ClojureScript support.

Show resolved Hide resolved src/orchard/cljs/analysis.cljc Outdated
@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Mar 29, 2019

Will update the README

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch 6 times, most recently from ad4c47b to c265970 Apr 4, 2019

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Apr 4, 2019

I added two new commits for trying some optimizations and cleaned up the PR.

I will as usually try this locally.

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch from d1c3f18 to cf3ef4c Apr 4, 2019

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Apr 15, 2019

Can you rebase on top of master?

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch from cf3ef4c to 180863a Apr 15, 2019

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Apr 15, 2019

@bbatsov Done

arichiardi added some commits Dec 8, 2018

Port ClojureScript info op from cljs-tooling
In order to start consolidating Clojure and ClojureScript tooling under our
apple orchard, this patch starts porting the info op from cljs-tooling.
Consequently, it paves the way to port some of the orchard Clojure-only code to
ClojureScript.
Merge see-also in the info function
This patch takes the see-also code from the info middleware, merging the
:see-also key in here together with the rest of the info map.
It also does a very important thing, which is to convert all the strings in the
see-also.edn file to symbols. This is better for readability and performance
now that we are 100% sure (see info/normalize-params) that we are only passing
symbols downstream.
Get rid of the orchard.meta/special-forms global
It was used only in one place and now that it is loaded dynamically it probably
makes sense to delay its loading for as long as we can.

@arichiardi arichiardi force-pushed the arichiardi:port-cljs-info branch from 180863a to a4ee30e Apr 15, 2019

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Apr 17, 2019

Small update - I'll merge this after @jeffvalk is ready with #20, as I want to cut one version of Orchard before we introduce such big changes.

@arichiardi

This comment has been minimized.

Copy link
Author

arichiardi commented Apr 17, 2019

@bbatsov Thank you for the update, really appreciate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.