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

Params should render themselves #16

Closed
wants to merge 6 commits into from

Conversation

erlend-aasland
Copy link

Based off of python#107623

erlend-aasland and others added 5 commits August 4, 2023 13:54
Extract helper methods for formatting the signature and parameter
sections, and clean up the remaining function body.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Previously, parameter docstring rendering was done in the DSLParser
class. Instead, let the Parameter class render themselves.
@erlend-aasland
Copy link
Author

I think this is pretty neat, @AlexWaygood: fa29529

@@ -2640,6 +2640,15 @@ def get_displayname(self, i: int) -> str:
else:
return f'"argument {i}"'

def render_docstring(self) -> str:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a good case for -> str | None

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH, I'm not sure it matters that much.

Copy link

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but I think we should go further. format_docstring() itself also looks like it should probably be a method on Function objects rather than a method on the DSLParser. It doesn't reference any state on the DSLParser, except for attributes on self.function. (This has implications for python#107623!)

So, instead of self.function.docstring = self.format_docstring() in the DSLParser.do_post_block_processing_cleanup() method, it would just be self.function.format_docstring() -- the Function object would know how to format its own docstring, instead of the DSLParser object reaching in and modifying the state of the Function object.

@erlend-aasland
Copy link
Author

This looks good, but I think we should go further. format_docstring() itself also looks like it should probably be a method on Function objects rather than a method on the DSLParser. It doesn't reference any state on the DSLParser, except for attributes on self.function. (This has implications for python#107623!)

Yes, that was my plan. I figured applying this change to Parameter was a nice start, since it makes for a small proof-of-concept PR.

So, instead of self.function.docstring = self.format_docstring() in the DSLParser.do_post_block_processing_cleanup() method, it would just be self.function.format_docstring() -- the Function object would know how to format its own docstring, instead of the DSLParser object reaching in and modifying the state of the Function object.

I'm not sure yet how to refactor the generation phase. I have a proof-of-concept branch locally (I might have pushed it here, I don't remember, and I don't intend to find out before finishing writing this :) ) where I ended up with something like this in render_function():

    template_dict['docstring'] = f.render_docstring()

@erlend-aasland
Copy link
Author

See pythongh-107790

@erlend-aasland erlend-aasland deleted the clinic/param-self-render-docstring branch August 8, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants