-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
Discern property variables #277
Conversation
This shows "property" if a variable is a property with a setter and/or a deleter and "ro-property" (read only property) if it is a property which does not have either a setter or getter.
OK, I do not understand the latest checks errors or how they are related with my changes, if at all. |
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.
Sorry for the delay. I was thinking about it. 😅
This will then need a simple unit test in pdoc.test.ApiTest.test_Variable_kind
.
pdoc/__init__.py
Outdated
if obj.fdel is not None or obj.fset is not None: | ||
vartype = "property" | ||
else: | ||
vartype = "ro-property" |
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.
How about we make 'property'
tentatively cover all kinds of properties. It's what Sphinx does (example, source) and it's what looks better to me. With Pythonistas undoubtedly more used to seeing properties as non-writable computed values, we should require writableness be mentioned in the docstring. Note, it's customary for only getters to have a docstring (cf. property()
help).
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 would really prefer some way of indicating this automatically. An alternative would be to indicate properties which do have a setter or deleter differently, e.g. as property/set or property/set/del though I like "ro" better as it is shorter. I find it strange to force humans to repeat themselves in all docstrings to document this rather than have the computer provide the information automatically.
And I would also rather submit a feature request ticket to Sphinx for this.
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, that's what I meant with tentatively: You raise the issue with Sphinx (or put it otherwise up for a vote), and then we see how to follow up, because it's easier to expand functionality than change / take it away abruptly. 😆
I must say I'm much more fond of property/set/del
. It shows most of my properties neatly as property
, and adds something extra for setters/deleters, just like Python source code does. 👍
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 have now changed this so that property is always followed by /get, /set, /del, and combinations thereof, depending on what the property can do, e.g. property/get/set, property/set etc.
(It is possible to create a property without a getter using the property() constructor, though not with a decorator, I think)
Also make property now show get/set/del depending on what is defined, e.g. propert/get/set
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.
Still missing a unit test.
pdoc/__init__.py
Outdated
if obj.fget is not None: | ||
kind += "/get" |
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 think even /set
and /del
are superfluous; how much more marking a getter. 😆 Pdoc3, as I see it, is about covering the 95% use case. Over 95% of Python properties define a getter (conjecture; happy to see it disproved). I'd strongly prefer properties with just the getter have more plain kind='property'
.
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 just think that if a program automatically shows the properties of a property, then it should do so in a logical, consistent and correct way. For anyone who wants to use an API, it is important information if a property can be read/written/deleted and this would show all of that. The other approach would be to show none and force people to document manually (and probably forget to do that) what can be determined automatically. Doing half of it would be even odder, because then it would not be self-obvious what is shown and somebody would first have to figure it out, and implementors would have to still manually document the excluded cases.
I am not sure what harms the additions would do at all: not much space is taken up and what is shown should be immediately obvious.
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.
For anyone who wants to use an API, it is important information if a property can be read/written/deleted
True. The only thing better than extra information is an API that is designed so clearly and intuitively that it prompts no additional tagging or guesswork. Properties are properties, on-the-fly computed values. Most properties can be read, a few properties can be set (with encapsulation, evidently, the sole primary use case), and if a property can be deleted, by god, you had better explained it to me why in prose!
Apologies to your case, but I've thought about it a lot and reached the conclusion that I want Variable.kind
be just plain 'property'
for now, with potential additions subject to what Sphinx community decides and what pdoc community decides (i.e. by way of new issue and its upvotes). 😳
I have pushed a seemingly innocuous change (the tests pass 😅) that exposes raw property/descriptor objects (instead of merely their getters) in pdoc.Variable.obj
. Thus, you can for the time being attach '-ro'
, '/get'
(etc.) suffixes in your overridden html template by e.g.:
if isinstance(var.obj, property):
if var.obj.fget:
...
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.
Fair enough! :) 👍
This shows "property" if a variable is a property with a setter
and/or a deleter and "ro-property" (read only property) if it is a property which
does not have either a setter or getter.
Fixes #276