-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refine for cljdoc/codox #34
Conversation
Now only documenting our public API
Might as well be explicit in what articles we want documented
If you want to preview what docs will look like in cljdoc, you can try the following: Terminal 1: launch a cljdoc server (leave this running)
Terminal 2: import lib into cljdocs
Browser: open http://localhost:8000/d/com.phronemophobic/membrane.term Note: This preview does compensate for any broken pom scm. Full cljdoc preview docs are here. |
@(requiring-resolve 'membrane.java2d/toolkit)) | ||
|
||
(def default-color-scheme | ||
(def ^:private default-color-scheme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it would be useful to make public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good observation. I shall make it so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to leave this one private and expose run-term and screenshots opts instead.
They both include default-color-scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to lean more on the open side for these types of things, especially if they're just data. What's the reason for not making this public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking:
- we are covered because it is included in both
default-run-term-opts
anddefault-screenshot-opts
- it is added to cljdoc docs, and thought it might be superfluous.
But... can easily expose it if that makes more sense to you. I have no strong feeling on the matter. If it will be useful to you or others, let's expose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phronmophobic lemme know your choice on this one, and I shall make it so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, it makes sense to expose it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good sir! It is done.
re: |
I think it might be a good hint if you remember what the values for the keys should be. |
Acts as documentation only. This provides useful reminders of possible opts keys in many IDEs.
- run-term options restated in screenshot docstring - toolkit option description corrected - options described in same order a they appear in fn signature
Ok @phronmophobic, took a shot at responding to your feedback, when you have time/interest, ready for next review. |
Cool. I'll take a look. Hopefully, these comments don't seem too nit-picky! |
Nah, I'm cool. I really enjoy learning from the perspective of others. |
Everything looks good to me 👍 . I'll just wait on making |
Ok, ready for another review and potentially a merge! |
Lemme know what you think. Details in commits.
Closes #28
(Does not include adding
:keys
destructuring to fn signatures as I felt these were noisy when reading docs on cljdoc, but you may want them still for other reasons).To complete the story for cljdoc we'll also likely want to address #33 so that we can get our
pom
scm
info correct.