-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Custom :rcparam: role. #10226
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
Custom :rcparam: role. #10226
Conversation
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.
conditional on the tests passing.
The biggest downside of this is that reading the docstrings via |
Hah, now you're going to be mad about that legend docstring check I snuck in.... If you change one, you have to change all three identically. Though I won't throw a fit if you remove the test. |
bah, I guess the test did its job... |
Yeah, but I'm not convinced this is so efficient. I'd still like to use something like the doc interpolation for things like this, assuming enough tools support them. |
I have a pretty good idea on how to make interpolation work robustly, but (you won't believe me) also a life outside of mpl these days... |
Oh, cool: cross-ref to #9414 But I like that syntax and the stricture that it has to stand on its own on the line. I don't buy the argument that our somewhat unique doc needs can't use a few non-standard tools so long as they are robust. pinging @timhoffm as I know he has been very interested in the state of the documentation. |
Can we just wait until we can use fstrings for the docs?
Good! |
When can we use f-strings? That seems like it’ll solve a lot of problems. But it’s a 3.6 feature right? Are we likely to drop <3.6 support at some point? |
f-strings are not recognized as docstrings... (which probably makes sense)
IIRC the plan is to make mpl 3 require py 3.6 (and there was much rejoicing). |
This looks cool. I'll give the folks who are interested in documentation a day or two to Approve before merging. |
I'm uneasy with the effect this has on plain-text readability; it seems like a possible net gain only if it makes a big difference on the Sphinx side. If I understand correctly, this is presently not the case. It does make writing a docstring slightly simpler, which is good, of course. |
Basically the problem is that there is no possible link target yet for these elements (no unified doc on the rcParams). I don't mind waiting for a proper rc docs be merged first (other than by the usual rebase churn). Because roles can also be in postfix position (with no change), an early version of this patch used
which may or may not considered an improvement. |
In general I consider @efiring 's concern a blocking issue... |
As mentioned above I don't have a particularly urgent need to get this merged. |
Readability is good, but is
harder to read than
let alone
The second does have the advantage of being a code snippet. |
That's the only point: code snippet versus something that has to be interpreted. As I said, I'm just a little uneasy--I'm not adamantly opposed, and maybe won't end up being opposed at all. Veering off the specific topic of this PR: Putting in sphinx markup involves a tradeoff, which needs to be evaluated on a case-by-case basis. For example, putting in the markup required to generate a link for a function can be worth the raw-text clutter once in a docstring; but if the docstring needs to use the name of the function more than once, there is no need to repeat the clutter. A single reference with a link is enough. |
+1 I think, the role is really helpful. Code snippet vs. something has to be interpreted
IMO worse than just
if there are concerns that this is not intelligible in plain text, one could change the role name to
OT: Evolution of docstrings From http://www.sphinx-doc.org/en/stable/ext/napoleon.html:
Yes, it's getting a bit longer. But since many docstrings are long anyway. it's better to add a clear structure at the cost of even a bit more length. If we use the short form If there is more to discuss OT, #10225 might be a better place. |
@efiring OK to merge? Its not that hard to search and replace to get rid of it again if we decide its not good.... |
There are two questions that need to be decided before it should be merged:
Modifying what the role does is relatively easy after this PR, creating the role, is merged; changing the name, or removing it entirely, is not. And question 2, naming, really does matter. I'm willing to go along with a "yes" for the first question.
I think (2) is better than (1) because it matches the relevant array name--maybe it is not a great name, but we are stuck with it.
Alternatives? Contrary arguments? I'm open to all. |
fwiw the original version I wrote used |
Who knows what I said two or three days ago, but I'm happy w/ any of these forms.... |
I think the role should be the same name as what appears in the rendered
docs. That would reduce the mental work needed for docstring writers. "rc"
is nice because it is next to impossible to confuse it with anything else.
"rcparam" or "rcParams" is likely to get mentally mixed up in people's
heads (namely, me).
…On Mon, Jan 15, 2018 at 2:03 PM, Jody Klymak ***@***.***> wrote:
Who knows what I said two or three days ago, but I'm happy w/ any of these
forms....
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10226 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-BytLZo2YeErcMHl5U3dAuoOdfBVks5tK6EZgaJpZM4RaPac>
.
|
I'm fine with either |
I also am fine with |
I think role names are actually case insensitive (but can't find the ref now and that should be checked again). |
Summarizing the opinions on naming:
No very strong opinions either way, but there's a clear tendency for |
fixed |
PR Summary
Make referencing rcParams in the docs a bit easier. See #10225 for example of motivation.
Note that this version doesn't actually create any link, but that could come as a followup.
PR Checklist