Skip to content

Developer Guide: Explain not to provide inline docstring when no __doc__ attribute and using the legacy script construct (#9943) - #9949

Merged
feerrenrut merged 4 commits into
nvaccess:masterfrom
accessolutions:i9943-developerGuideScriptDocstring
Dec 9, 2020
Merged

Developer Guide: Explain not to provide inline docstring when no __doc__ attribute and using the legacy script construct (#9943)#9949
feerrenrut merged 4 commits into
nvaccess:masterfrom
accessolutions:i9943-developerGuideScriptDocstring

Conversation

@JulienCochuyt

@JulienCochuyt JulienCochuyt commented Jul 17, 2019

Copy link
Copy Markdown
Contributor

Link to issue number:

Related to #9943

Summary of the issue:

When using the legacy script construct, providing an inline docstring and no __doc__ attribute sets the docstring as the input help mode message for the script.
The message set that way is not translatable.

Description of how this pull request fixes the issue:

Explain in the Developer Guide that inline docstrings are fine only when using the new @script decorator.

EDIT:
Additionally, replace "function" with "method", as scripts do require to be methods and not bare functions.

Testing performed:

None

Known issues with pull request:

Change log entry:

IMHO, no special announcement is needed, as the @script decorator is the new recommended pattern anyway.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

I think you can make an inline doc string translatable by wrapping it in a _(...) call.

@JulienCochuyt

JulienCochuyt commented Jul 18, 2019

Copy link
Copy Markdown
Contributor Author

I think you can make an inline doc string translatable by wrapping it in a _(...) call.

That would indeed be nice, but according to PEP 257, you actually can't:

A docstring is a string literal that occurs as the first statement in a module, function, class, or method definition. Such a docstring becomes the __doc__ special attribute of that object.

"string literal" is the key here.

To be sure, I just checked on a global plugin script.

Btw., the Developer Guide also describes the scripts as functions, while they are required to be methods.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Btw., the Developer Guide also describes the scripts as functions, while they are required to be methods.

The distinction between functions and methods has always been a bit vague here. There are cases where a lambda is used as a script, however in the end, it ends up being a method. So I agree here.

@JulienCochuyt
JulienCochuyt force-pushed the i9943-developerGuideScriptDocstring branch from 6a60751 to 17b8d12 Compare August 13, 2019 12:15
@JulienCochuyt

Copy link
Copy Markdown
Contributor Author

Rebased onto latest master.

@LeonarddeR, I am not sure if I understand your last comment correctly: Do you agree with my second revision or with the original statement and would prefer I revert my last change?

@feerrenrut

Copy link
Copy Markdown
Contributor

I think we could make it clearer that new code should all use the script approach. It has been available for long enough, and used widely now.

It could be reworded to with an emphasis on "This is the way to do it." then underneath, in the past it was done this way, which had these problems / gotchas.

@lukaszgo1

Copy link
Copy Markdown
Contributor

@JulienCochuyt Is this still on your radar?

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @JulienCochuyt.

I think we should take this as-is. There are lots more clarifications we could make, and I'll be happy to take them as separate PR's

@feerrenrut
feerrenrut merged commit 2030dd6 into nvaccess:master Dec 9, 2020
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Dec 9, 2020
@JulienCochuyt
JulienCochuyt deleted the i9943-developerGuideScriptDocstring branch December 9, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants