Trust ClojureScript :file analysis metadata

[lread]

Updated ClojureScript's reader to use the :file metadata returned by analysis.
It formerly set :file to the actual path of the :file it was analysing. This
change matches the Clojure reader behaviour and allows for potemkin import-vars
type manipulation of metadata to, for example, point at an alternate var's docs
in a different source file.

To verify readers are still returning data that we expect, I added some tests
based on the existing playgound under test-sources. The tests are a good start
but not comprehensive and should be extended as needed. Circleci config
is included to automatically run tests on commit.

While adding tests, I noticed small bugs and made corrections:

1. The ClojureScript reader now sorts protocol methods by name. This
   matches the behaviour of the Clojure reader.
2. The Clojure reader will now add a single var for each defrecord
   with the name of the defrecord. This matches the ClojureScript
   behaviour and is apparently the desired behaviour for cljdoc.

For consistency and testability:

• we are now removing keys with empty/nil values from reader analysis maps.
• to be conistent with the ClojureScript reader, the Clojure reader no longer
  returns :file :line :column :end-column and :end-line on the namespace
  element.

For maintainability and to reduce confusion I removed unused: html writer,
resources and project.clj.

Notes:

As part of this change, the ClojureScript reader will stop supressing protocol
functions when the actual source does not match the :file metadata. This allows
for a tool such as import-vars to point to, and have documented, an imported
protocol function. No action was needed for the Clojure reader to make this
work.

This change was made a bit challenging due to cljdoc's usage of codox's
root-dir and :file metadata:

The original intent of codox is that the config :root-dir point to the
project (aka git) root of a project and that the :file metadata
was simply what was returned by analysis. Codox added the :path to analysis
metadata to resolve :file to the :root-dir.

Cljdoc does not set the :root-dir to the project root, ignores the :path and
assumes that the :file is always relative to configured :source-path. Although
this usage works for cljdoc, it did sprain my brain while making this change.

To compensate, I have been careful to ensure that :file continues to always
be relative to the the configured :source-path.

Another oddity was that altered :file metadata is being resolved to paths
within the jar file when codox is called from cljdoc. This might be due to the
way the classpath is setup when doing an analysis run. My change is coded to
handle this situation when it arises.

[lread]

It was a little more complicated than I expected, but I think I've got it.

My manual testing was with a version of cljdoc that pointed to my altered codox. I could, of course, only test locally. I tested with my work in progress of rewrite-cljs which alters metadata for cljs and clj and compared analysis output with current version of codox to what my altered version generates. I also checked that results in my browser looked good.

Don't be shy to critique, I am happy to conform to what makes sense for your project.

[martinklepsch]

Hey Lou! First of all thanks for the work you did on this, the tests look great and this certainly will be helpful maintaining this project going forward! 👍

While the code makes sense I'm not 100% sure it's backwards compatible with Codox and so it might make sense to pull the Codox reading code into a fork that's more aimed at extracting data about Clojure namespaces for programmatic consumption. A new name, maybe cljdoc/analyzer, could make sense too.

One thing I noticed is that the :path = :file tests fail when you don't provide a :root-path option. Not sure if this is intended or not.

Another thing I was wondering was the following situation:

1. We have multiple :source-paths, e.g. ["/some/dir/a/test" "/some/dir/b/main"]
2. Some import var style magic "copies" a var from one of these to the other
3. Would the :file value still end up being correct?

Admittedly this is a contrived example and not super important to completely get right.

While this shouldn't be part of this PR I was also wondering if a bigger refactoring would make sense in which the two core steps of the analyser are split up more explicitly:

1. Find namespaces in provided source dirs
2. Get metadata for those namespaces and it's public vars

I think especially step one could be done better, sharing more code between the Clojure and ClojureScript implementations.

Final question: did you test this with cljdoc? I'm a little unsure about the effect of :root-path and all that so before we merge that's probably worth looking into as well.

[lread]

Hey Lou! First of all thanks for the work you did on this, the tests look great and this certainly will be helpful maintaining this project going forward! 👍

Thanks for taking time to review, Martin! And don’t get me wrong, I am sure Lou is great, but my name is Lee. :-)

While the code makes sense I'm not 100% sure it's backwards compatible with Codox and so it might make sense to pull the Codox reading code into a fork that's more aimed at extracting data about Clojure namespaces for programmatic consumption. A new name, maybe cljdoc/analyzer, could make sense too.

Although cljdoc’s codox is diverging away from codox in general, I am thinking this change is likely codox compatible. Another option would be to do this work in codox master, if there is interest over there, and merge it back here. Let me know what you think.

One thing I noticed is that the :path = :file tests fail when you don't provide a :root-path option. Not sure if this is intended or not.

Yes, cljdoc usage of :root-path does not match codox master intended usage; cljdoc usage essentially breaks the :path which cljdoc ignores anyway, so, why did I check it? My test here was to try to verify that my changes did not break intended usage.

Another thing I was wondering was the following situation:

1. We have multiple :source-paths, e.g. ["/some/dir/a/test" "/some/dir/b/main"]
2. Some import var style magic "copies" a var from one of these to the other
3. Would the :file value still end up being correct?

Admittedly this is a contrived example and not super important to completely get right.

Interesting question. As cljdoc currently only uses one source-path, I did not cover this scenario under tests. I would have to add tests to be sure this scenario is covered for both existing clojure import-vars magic and the new clojurescript support I added. I think it might be but am not sure. Let me know if you want it covered. That said, because cljdoc analyzes source from the jar, are multiple source paths likely?

While this shouldn't be part of this PR I was also wondering if a bigger refactoring would make sense in which the two core steps of the analyser are split up more explicitly:

1. Find namespaces in provided source dirs
2. Get metadata for those namespaces and it's public vars

I think especially step one could be done better, sharing more code between the Clojure and ClojureScript implementations.

Yes, I expect more code sharing is possible, but agree a separate PR makes sense if we decided it was worth pursuing this. This, of course, would be a further divergence away from codox master.

Final question: did you test this with cljdoc? I'm a little unsure about the effect of :root-path and all that so before we merge that's probably worth looking into as well.

Yes, please see the separate comment I added to the PR explaining how I tested with cljdoc.

[martinklepsch]

Thanks for taking time to review, Martin! And don’t get me wrong, I am sure Lou is great, but my name is Lee. :-)

OMG, so sorry 🤦‍♂️ Wrote this on a plane w/o internet and it was what came to mind but then forgot to check before sending. Oops 🙈

That said, because cljdoc analyzes source from the jar, are multiple source paths likely?

For cljdoc that's unlikely but other Codox users might do something like that I guess.

Another option would be to do this work in codox master, if there is interest over there, and merge it back here.

There's been a long-standing PR to incorporate some of the improvements I made to Codox back into it: weavejester#179. Unfortunately that hasn't been merged yet. I think the lack of tests makes this kind of change a bit frightening to merge for Codox' maintainers so maybe your changes would make that a little more appealing. We should try to get in touch with @weavejester and hash something out. I'd love for these two projects to converge again. But then again we've also introduced some separate build tools and all that now so we'll see how that goes.

Yes, cljdoc usage of :root-path does not match codox master intended usage;

Hm, as far as I understood cljdoc uses Codox' default value for :root-path which is (System/getProperty "user.dir")?

I'll want to give this another test run before merging but other than that it sounds like it's good to go! 👍

[weavejester]

There's been a long-standing PR to incorporate some of the improvements I made to Codox back into it: weavejester#179. Unfortunately that hasn't been merged yet. I think the lack of tests makes this kind of change a bit frightening to merge for Codox' maintainers so maybe your changes would make that a little more appealing.

The lack of tests are fine, as a visual inspection of the documentation output is always required. The reason it hasn't been merged is because while the improvements are very useful, they're all mixed together along with a bunch of other changes that aren't relevant.

If someone wants to pull out the useful parts of weavejester#179 into clean PRs, then I'd be happy to merge in the improvements, but I just haven't had the time to do it myself.

[martinklepsch]

@weavejester Thanks for the update, I was thinking that it might be easy enough to clone the branch and cherry pick the commits I mentioned in the PR description. If that process seems too tedious I could also produce some PRs — would you like to get each commit in a separate PR?

[weavejester]

If you can spare the time, that would be a big help. Unless there are multiple commits contributing to a single piece of functionality, a PR per commit sounds fine. The commit messages will also need to be amended slightly so they adhere to the contributing guidelines.

[lread]

Thanks for taking time to review, Martin! And don’t get me wrong, I am sure Lou is great, but my name is Lee. :-)

OMG, so sorry 🤦‍♂️ Wrote this on a plane w/o internet and it was what came to mind but then forgot to check before sending. Oops 🙈

No problemo! You just probably got me mixed up with Lou Reed. 😎

Yes, cljdoc usage of :root-path does not match codox master intended usage;

Hm, as far as I understood cljdoc uses Codox' default value for :root-path which is (System/getProperty "user.dir")?

Hmm... where the heck did I get this unabashed confidence about :root-path? The default is indeed the directory from which java was launched (user.dir). There is no mention of :root-path in the the codox README... oh it is the codox code that told me... read-namespaces calls add-source-paths with the root-path, and add-source-paths assumes that root is the project root dir.

I was thinking to myself, why the heck doesn't Martin understand? And now I know why, I completely forgot that the details are hidden in the code!

So for cljdoc's usage the :root-path is the current directory, but this is not the project root, hence :path not being resolved correctly for cljdoc. Have I got that right?

In fairness to codox master, since it is normally used from lein or boot, an incorrect :root-path would not likely be an issue for its users.

[lread]

If you can spare the time, that would be a big help. Unless there are multiple commits contributing to a single piece of functionality, a PR per commit sounds fine. The commit messages will also need to be amended slightly so they adhere to the contributing guidelines.

I think that this particular PR might have to to be split to be @weavejester approved ™️? For this PR, the unit tests did help me to see issues I would not have with a visual inspection and I would recommend including them should this work be brought over to codox master.

[lread]

@martinklepsch, I ask only because I don't know :-)

1. we chatted about many things here, were there any further action items for this PR? I do think merging back to codox master is interesting but a separate discussion. Should we open a separate issue here to discuss?
2. did my explanation :root-path make sense to you?

[martinklepsch]

@lread no further actions required, I've done some further modifications and will likely move further away from codox[1]. I'll ping you for feedback once this is ready.

[1] All what we're using is really only a small layer over the native clj/s tooling and maintaining API compatibility with codox (which has a much bigger scope) has become more of a burden than something we actually get a lot of value out of. Maybe at some point whatever this will become could serve as a foundation for Codox's rendering. We'll see.

[lread]

Thanks @martinklepsch, sounds good!