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

[alt] Refactor DSLParser.format_docstring() #18

Closed
wants to merge 3 commits into from

Conversation

AlexWaygood
Copy link

@AlexWaygood AlexWaygood commented Aug 5, 2023

Turn it into a method on Function objects

@erlend-aasland, what do you think of this approach, as an alternative to python#107623 + #16? It gets rid of more code ;)

Turn it into a method on `Function` objects

Co-authored-by: Erlend E. Aasland <erlend@python.org>
@AlexWaygood AlexWaygood changed the title Refactor DSLParser.format_docstring() [alt] Refactor DSLParser.format_docstring() Aug 5, 2023
@erlend-aasland
Copy link

@erlend-aasland, what do you think of this approach, as an alternative to python#107623 + #16? It gets rid of more code ;)

AFAICS, this only takes both of those PRs a step further :) Makes sense to me. I think we could start with #16, then python#107623, and then apply the changes from this PR.

@AlexWaygood
Copy link
Author

AFAICS, this only takes both of those PRs a step further :)

Yes, exactly!

I think we could start with #16, then python#107623, and then apply the changes from this PR.

Hmm... I guess we just disagree on process, then :)

For me, python#107623 looks kinda odd as-is, because you're making all these new @staticmethods... which aren't really static at all! They're pretty stateful, it's just that it's not the DSLParser state that they're accessing and modifying -- it's the state of the DSLParser.function attribute that they're accessing and modifying. For me, the diff makes much more sense -- and is actually more readable -- if we do it all in one go, like this PR?

@erlend-aasland
Copy link

Hmm... I guess we just disagree on process, then :)

For me, python#107623 looks kinda odd as-is, because you're making all these new @staticmethods... which aren't really static at all! They're pretty stateful, it's just that it's not the DSLParser state that they're accessing and modifying -- it's the state of the DSLParser.function attribute that they're accessing and modifying. For me, the diff makes much more sense -- and is actually more readable -- if we do it all in one go, like this PR?

After Serhiy's @text_signature, there's only one @staticmethod left in that PR. I would like to break it up in multiple PRs, since it is easier to bisect in case a bug sneaks in.

@AlexWaygood
Copy link
Author

See python#107840

@AlexWaygood AlexWaygood deleted the format-docstring-redux branch August 10, 2023 20:45
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