Skip to content
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

Non-__doc__ document support #138

Closed
tkf opened this issue Feb 23, 2013 · 18 comments
Closed

Non-__doc__ document support #138

tkf opened this issue Feb 23, 2013 · 18 comments
Labels

Comments

@tkf
Copy link
Contributor

tkf commented Feb 23, 2013

I suggest to support "attribute docstrings" and "additional docstrings" mentioned by PEP 257:

String literals occurring elsewhere in Python code may also act as documentation. They are not recognized by the Python bytecode compiler and are not accessible as runtime object attributes (i.e. not assigned to __doc__), but two types of extra docstrings may be extracted by software tools:

  1. String literals occurring immediately after a simple assignment at the top level of a module, class, or __init__ method are called "attribute docstrings".
  2. String literals occurring immediately after another docstring are called "additional docstrings".

-- PEP 257 -- Docstring Conventions

Sphinx autodoc also support fetching document from comment started by #:. This may be useful for some people.
http://sphinx-doc.org/ext/autodoc.html#directive-autoattribute

I may be going to work on this issue at some point but feel free to start working on it if someone wants to do that. Also, advices on where to start is very welcome.

@davidhalter
Copy link
Owner

Oh just try! I'm still trying to improve documentation (it should be easier to do something like this).

Basically you just need to alter the parser (parsing.py) and add a docstring attribute to parsing_representation.Statement. Just try :-) If something's to hard we should probably make the whole thing easier to understand (just let me know if you don't understand something!!).

@tkf
Copy link
Contributor Author

tkf commented Feb 23, 2013

I thought I'd do it in BaseDefinition.doc. If the object has no __doc__ then look at the next statement and see if it is a string literal and use it if so. Though I don't know if it works because I don't know if string literals not at RHS are stored in the parse tree. What do you think? Is it better to do it at parse-time?

@davidhalter
Copy link
Owner

I would do it at parse-time, because that's just the cleaner way.

@davidhalter
Copy link
Owner

I disabled this a while ago, because goto_definition didn't seem the right place for documentation stuff. We should be adding a documentation command that returns a Documentation object with all docstrings found. The evaluator can gather those.

@tkf
Copy link
Contributor Author

tkf commented Mar 13, 2014

Consider:

some_var = object()
"""An object with unique id for XXX"""

This is what I call definition of some_var and thus where goto_definition has to go. This is why I did that, IIRC.

@davidhalter
Copy link
Owner

Well. goto_definition goes to the object definition :-) That's not possible obviously (since object is a builtin).

There are quite a lot of confusions around goto_definition. But it's not your typical goto function. That's what goto_assignments is. That being said, I know some people don't like it, because it doesn't follow imports. That's true, but we still can:

  1. change that behaviour of goto_assignments
  2. or even better, create a param follow_imports=True

If you really think the name goto_definition is completely wrong, we could think about calling it types or something like that (and switch back to calling goto_definition, gotos or something.

@tkf
Copy link
Contributor Author

tkf commented Mar 13, 2014

You can have callable factory CallaebleFactory which makes tons of callables. These callables can behave differently depending on parameter passed.

callable_a = CallaebleFactory(a=1, b=2, c=3)
"""<docstring>"""

callable_b = CallaebleFactory(a=4, b=5, c=6)
"""<docstring>"""

callable_c = CallaebleFactory(a=7, b=8, c=9)
"""<docstring>"""

In this case, what you want to know is the parameters passed to CallaebleFactory because the parameters define how callable_x works. Goto the definition of CallaebleFactory is useless since there could be bunch of callables using the same CallaebleFactory and you cannot tell why and how callable_a and callable_b behave just by looking at CallaebleFactory.__call__. If an assignment is documented with docstring, then there is very high probability that assignment is the definition of that "constant". (Note that this holds not only for CallaebleFactory. I can imagine other cases but I thought CallaebleFactory is the best example.)

If you really think the name goto_definition is completely wrong, we could think about calling it types or something like that (and switch back to calling goto_definition, gotos or something.

I think the name and concept you have in mind is "half" wrong ;) There should be some function that finds (or guesses) the place of "semantic" definition of a variable (we can try to make what we mean by "semantic definition" well defined by using docstring etc. as criteria like I do above). And calling it goto_definitions makes sense. So, no, I don't think we should remove goto_definitions. At the same time, I can imagine function that finds only the definition of variable's type could be useful. So, yes, I think it is a good idea to have types (goto_types?).

@davidhalter
Copy link
Owner

So something like:

def goto_assignments(self, follow_imports=True)
def goto_definitions(self)
def goto_types(self)

would sound reasonable? goto_types would be the current goto_definitions function and goto_definitions would be some magical do something goto :-) However, I agree that this definition would be semantically more correct.

The question is if there's really much use for this if we introduce follow_imports=True. At this point we would also need to ask the question if it's really worth the trouble, because rarely an IDE is going to use all of those functions (also almost nobody is using attribute docstrings, not that that's a reason not to do it, but still it might not be worth the struggle).

And BTW: I also removed your functionality mostly because it introduced a number of bugs. It was a quick hack (3 lines of code?), didn't work correctly for certain nested conditions.

@tkf
Copy link
Contributor Author

tkf commented Mar 13, 2014

The question is if there's really much use for this if we introduce follow_imports=True.

That is a good question. You can have something like from .submodule import something in yourmodule/__init__.py. Then you can use it as from yourmodule import something from yourscript.py. Then in that case it makes sense to have a method that skips from ... import ... stuff. I don't know the exact definition of follow_imports but I assume this is plural since your intention is to skip all of these so you may say in that case goto_assignments(follow_imports=True) and goto_definitions2() (let's put "2" at the end to avoid confusion with the current version of goto_definitions) are the same. However, you can have complicated assignments rather than imports in yourmodule/__init__.py. So, if goto_assignments has follow_assignments instead of follow_imports, then it is maybe equivalent to goto_definitions2. But I am not 100% sure and have to think about it.

BTW, if goto_types is introduced, when you hit it on a variable assigned to a class (rather than an instance), then it jumps to its metaclass, right? Hmm.... I start liking the idea ;)

rarely an IDE is going to use all of those functions

Then I think most of time people use goto_definitions2, since this is the most magical one.

almost nobody is using attribute docstrings

You mean "nobody around you" ;) I think I see it place to place and I'd guess it won't be implemented otherwise. But of course I could be biased since I am using it. If we want to not implement it because "nobody use it" then we should check it if it is true in popular projects. But I'd say it is generally a good idea to make things PEP compatible.

Also remember that we can use criteria other than docstring. For example, something like "if top level variable is defined by a function call (or instantiation of a class) then that call is the definition."

I also removed your functionality mostly because it introduced a number of bugs.

Oops sorry about that. Which functionality? I understand stability is more important so your choice is reasonable.

@davidhalter
Copy link
Owner

I don't know the exact definition of follow_imports but I assume

It doesn't exist yet, it's just an idea I've wanted to share.

But I am not 100% sure and have to think about it.

Yes that's my stance, too. I don't think we should be too quick changing the behavior again, since we've done that two versions again. But the discussion is definitely important.

Then I think most of time people use goto_definitions2, since this is the most magical one.

No IDE's will probably always use goto_assignments (with certain params we don't have yet) and ignore everything else, because that's the normal goto function. I haven't seen any other occurrences goto in IDEs, but I also rarely use them. I.e. any feedback on this subject is very welcome.

BTW, if goto_types is introduced, when you hit it on a variable assigned to a class (rather than an instance), then it jumps to its metaclass, right? Hmm.... I start liking the idea ;)

Hehe. It's not the type method we're talking about ;-) And how confusing would that be :-D

Oops sorry about that. Which functionality? I understand stability is more important so your choice is reasonable.

Two lines of code in evaluate/__init__.py and a few tests that I would like to reintroduce, it's basically your PR of #141.

But I'd say it is generally a good idea to make things PEP compatible.

Yes it is, but PEP #257 doesn't really describe Attribute Docstrings. It's really not about that, it's about describing how to write docstrings. #258 or #224 would describe Attribute Docstrings, but they were both rejected. It's not something that the compiler recognizes or anything else of the standard library. The only reason why people are using "Attribute Docstrings" is the fact that Sphinx is recognizing them and it works very well to document them.

Sphinx is big and we should care about it, but we could also check #: comments, which Sphinx also leverages. I'm not arguing against you, I'm just saying it's really not about the PEPs, because they don't describe it. It's about the ecosystem. I'm very afraid of using this magic in goto_definitions, because once we're starting like that, it's only going to get worse and worse.

@tkf
Copy link
Contributor Author

tkf commented Mar 13, 2014

I don't think we should be too quick changing the behavior again, since we've done that two versions again. But the discussion is definitely important.

Agree!

Hehe. It's not the type method we're talking about ;-) And how confusing would that be :-D

Why? If we do this in current goto_definitions then that would be confusing. But if the name of the command is goto_types and if you understand Python's type system then that should not be confusing since the type of type is metaclass.

But I agree that it may not be useful all the time. And I think this is why we should have goto_definitions2. We have concisely defined methods goto_assignments and goto_types. In between, we have "practically" useful goto_definitions2.

Yes it is, but PEP #257 doesn't really describe Attribute Docstrings. It's really not about that, it's about describing how to write docstrings

Yes it does. What about the part I quoted in the very beginning of this issue (see above)? I didn't know both PEP258 and 224. Since it looks like PEP258 is "in" docutils and docutils (+Sphinx) is practically official docstring processor even though it is not in stdlib, I think we should assume attribute docstring is also "practically official". Besides, as I said, attribute docstring is described in PEP-257 which is not rejected.

Sphinx is big and we should care about it, but we could also check #: comments, which Sphinx also leverages.

Yea, I agree. I just didn't implement it since implementing attribute docstring was already some work. (BTW I don't like #: since it violates PEP8. It is OK syntax but I also think they could find something PEP8 compatible if they just try harder...)

I'm just saying it's really not about the PEPs, because they don't describe it. It's about the ecosystem.

It is about PEP here. But I agree about your argument about ecosystem.

@davidhalter
Copy link
Owner

Yes it is, but PEP #257 doesn't really describe Attribute Docstrings. It's really not about that, it's about describing how to write docstrings

Yes it does. What about the part I quoted in the very beginning of this issue

The only thing it says is that we call strings after statements "attribute docstrings" and even there it's in quotes. They don't give "attribute docstrings" any other meaning than "may also act as documentation". So obviously Jedi could (doesn't necessarily have to) use those docstrings for documentation purposes.

This being said, we should really do some research in well known Python projects and check how much Attribute Docstrings are really used - and more importantly - where they are used. If the main use case is describing attributes for sphinx and documentation of "important" definitions, you might have a point.

@tkf
Copy link
Contributor Author

tkf commented Mar 13, 2014

OK so you mean "PEP257 does not says Python helper tools have to parse Attribute Docstrings. Thus, not parsing Attribute Docstrings does not imply PEP257 incompatibility." Do I understand you correctly? Then, right, PEP257 does not says it is mandatory to parse it. In PEP257, parsing Attribute Docstrings is an optional specification. Maybe I should have said "it is not PEP complete" or something. Anyway this whole paragraph is a nit-picking ;)

do some research in well known Python projects and check how much Attribute Docstrings are really used

Agree, but that would be hard. The easiest way is to ask (poll?) people, I guess. But I don't know the best place. It's much easier if Jedi has an ML.

BTW, it looks like pocoo guys are using #: style comment. See the bottom of:
http://www.pocoo.org/internal/styleguide/

@tkf
Copy link
Contributor Author

tkf commented Mar 13, 2014

We've been discussing about two different things. (1) goto_definitions vs goto_types; (2) attribute docstrings. As I said, (1) does not depend on (2), although (2) could be a good criteria (just my guess). But It looks like we don't have an issue for (1) yet. Shall I add one?

@davidhalter
Copy link
Owner

pocoo guys are using #: style comment.

e.g. https://github.com/mitsuhiko/flask/blob/master/flask/helpers.py#L769

Shall I add one?

Yes, why not.

It's much easier if Jedi has an ML.

What's an ML?

@tkf
Copy link
Contributor Author

tkf commented Mar 14, 2014

I thought there may be some discussion already.

ML = mailing list

@erichiller
Copy link

variable = value #: definition
or

#: definition
variable = value

style Documentation is exactly what I've been looking for!

@davidhalter
Copy link
Owner

Attribute docstrings are now supported.

It's been a while, but thanks again for the discussions @tkf. It helped a lot back in the days, especially the testing improvement pull requests you made.

If you're ever back to the Python world, you're definitely very welcome to contribute again :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants