Skip to content

Conversation

@ptaoussanis
Copy link

Fixes #216 and includes all fixes from #179 (these are relevant here).

Live examples of the current cross-platform output:

High-level notes

  • Please see individual commits for more info.
  • I've tried to match project conventions for commit and code style.
  • Just ping if you'd like any changes.
  • Likewise, I'm happy to remove the commits from Various Enhancements #179 if you'd prefer to handle those separately. I've combined them here for your convenience.
  • No changes to README are included. Happy to add those if you're otherwise satisfied with the PR.
  • This is intended to be a purely additive PR- i.e. is intended to not break anyone using the new features.

Implementation notes

  • The changes related to {:language :both} support should not affect those using {:language :clojure} or {:language :clojurescript}.
  • An optional mechanism is provided for advanced users that want to avoid broken links when upgrading a project from single platform to cross-platform.
  • This implementation does (ab)use the internal project map for conveying relevant internal state. I'm satisfied with this approach, but drawing attention to it in case you feel differently.
  • I don't personally make use of the html/write-documents code, and haven't tested it specifically - but I see no reason why this PR should cause any problems with it.

Thanks a lot for your time & effort.
I know big PRs like this can be time-consuming to properly review. Just ping if I can answer any questions, etc.

Cheers :-)

martinklepsch and others added 8 commits July 5, 2023 14:33
Author: @martinklepsch

Codox previously did two passes to also read macros for ClojureScript.
This is actually no longer necessary with more recent versions of ClojureScript.

cljdoc@3079986
Author: @martinklepsch

For some reason the list? check wasn't enough to remove 'quote and I ended up with
'quote in arglists. seq? did the trick (and might be more appropriate anyways?).

cljdoc@25b01ae
Author: @martinklepsch

I noticed some issues (+ more details[2]) with the new CLJS version.
Turns out setting *analyze-deps* to false was never really supported for
macro-containing namespaces and we just got lucky[3].

Fixes [1]

[1] cljdoc/cljdoc#201
[2] https://gist.github.com/martinklepsch/9f885feb061ec3f03f365e22d0d9bf5b
[3] https://clojurians-log.clojureverse.org/cljs-dev/2018-11-09/1541786231.245300

cljdoc@e0cd269
Author: @martinklepsch

When there are multiple files (e.g. .clj and .cljc) for a namespace
Codox would analyse those namespaces twice returning the analysis
result twice as well.

cljdoc@3f2ea84
cljdoc/cljdoc#155
Author: @ptaoussanis

- This implementation attempts to be minimally invasive.
  Most of the logical changes are within `html/write-index` and
  `html/write-namespaces`, where a special path is introduced
  for "cross-platform" projects (= language `:both`).

- NO behavioural changes are intended for traditional
  (non-cross-platform) projects.

- See weavejester#216 for detailed feature discussion.
Author: @ptaoussanis

This is an advanced option to help prevent any broken doc links
when upgrading a project's docs from single language to dual language.

In this case, :base-language can be set to the previous (single)
language for which doc links may already exist in the wild.

In detail, if {:language :both} then:

  {:base-language nil}            => ".clj", ".cljs" file extensions
  {:base-language :clojure}       => nil,    ".cljs" file extensions
  {:base-language :clojurescript} => ".clj", nil     file extensions

For example:

  Library Foo previously used {:language :clojure} (either because
  it was Clojure only, or because of limitations in Codox).

  Various links to Foo's Codox documentation now exist in the wild.

  Foo's authors want to change to {:language :both}, but don't
  want to break pre-existing links in the wild.

  In this case, Foo's authors can use the following opts:
    {:language :both
     :base-language :clojure}

  This will produce files like the following:
    com.foolib.html      ; For Clojure       platform
    com.foolib.cljs.html ; For ClojureScript platform

  Any pre-existing links will successfully point to the same
  (Clojure) docs they did previously.
Author: @ptaoussanis

Note that this commit introduces new classes to `default.css` used (only)
by "cross-platform" projects.

This means:

  - NO behavioural changes are intended for non-cross-platform projects.
  - Cross-platform projects WILL require a theme that includes the
    new CSS classes.
Author: @ptaoussanis

Small change that hopefully contributes to a more modern look
@weavejester
Copy link
Owner

Thanks for the PR! I think we'll need to break this up to be a little to be manageable:

PR 1. Fix ClojureScript issues: df7ddcb, 370ef9e and ebfece3.
PR 2. Fix repeated namespace parsing: 492837f. (This is separate from PR 1, as it seems sufficiently different).
PR 3. This PR: 732ddaa, ce96de8 and 0906e86.
PR 4: UI appearance improvement: f5e4b62. (This seems independent from this PR).

If this sounds like too much of a pain, I can handle the first two PRs myself, as I probably should have done that a while ago.

Rather than {:language :both}, should we future-proof this by allowing a set or collection of languages instead? :both could be obsolete for, say, documenting Clojure and Babashka namespaces.

This was referenced Jul 6, 2023
@ptaoussanis
Copy link
Author

👍 Closing, this work is now spread across the following PRs:

Rather than {:language :both}, should we future-proof this by allowing a set or collection of languages instead?

Have updated the relevant code 👍

@ptaoussanis ptaoussanis closed this Jul 6, 2023
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.

[Feature request] Support simultaneous Clojure & ClojureScript output

3 participants