Layout a docstring parser base

[pawamoy]

Related to #25

The idea is to lay things out for allowing contributors to implement different docstrings parsers more easily.

[shyamd]

If I look at Google implementation, I don't see any use for those book-keeping attributes you set in Parser.__init__. I want to make sure the Parser isn't holding part of the state when it doesn't need to. That's how we get state bleed and weird bugs. Parser doesn't need an __init__ from what I can see.

[pawamoy]

The Google parser does use these attributes: _path, _errors, _signature and _return_type are all used throughout most of the methods (parse, read_parameters_section, read_exceptions_section and read_return_section).

I'd like to get rid of this "state" but I don't see how 😕

[shyamd]

Then the parse command should clean up that state at the end. I don't mind having Parser level state objects but those should be more generic. Maybe a set of all errors the Parser has encountered kind of thing?

[pawamoy]

Then the parse command should clean up that state at the end.

I do that at the beginning of do_parse instead.

Maybe I should simply remove do_parse, and make parse return the errors itself.
But that still doesn't solve the need to store the path, signature and return type, which will be needed for other parsers as well. Maybe helper methods could hide that away.

def parse(docstring, path, signature, return_type):
    self.set_state(path, signature, return_type)
    # do stuff, use self.state.signature and self.state.return_type as needed

    # handle error, self.record_error will prefix with self.state.path
    self.record_error("message")

    # at the end
    return sections, self.pop_state()

Maybe a set of all errors the Parser has encountered kind of thing?

This what _errors is used for. Not sure to understand what you mean 🙂

[pawamoy]

Another solution would be use no state at all, and pass all the arguments through every method.

[shyamd]

If you have to do that, move those state variables to Google. I think the biggest issue is that Parser is an abstract class that is pretending to have an implementation in it. Those state variables right now are only relevant to the Google implementation.

[pawamoy]

I thought it would be convenient to give access to the object path, signature and return type to any parser implementation 😅

I'll see what I can do. Don't hesitate to open a PR based on this branch if you want to give it a try as well 🙂

In any case, thank you for your help!

[pawamoy]

Sorry but I don't see how I'm supposed to get rid of the state in the base parser class.

The google parser needs the object path, signature and type, so these values must be passed from Object.parse_all_docstring, which does not know which type of parser it is. It means that if I accept these values in the google parser instead of the base one, every other parser will have to accept them as well. In that case, what's the point of a base class? We could simply let contributors write the whole parsers themselves again and again, freely.

Besides, I think every parser will benefit from having access to the object signature and type, and the object path is required anyway for consistent error reporting.

I will keep the base parser structure like that. I think it makes it easy to write a parser implementation.

[shyamd]

Yeah, Let's keep it as is right now. I can make an issue or PR to demonstrate what I mean.

[pawamoy]

Alright, thanks 🙂