spadarian / docblock-python

Atom plugin to insert documentation blocks for python functions
GNU General Public License v2.0
22 stars 9 forks source link

Decouple code parsing, docstring formatting, and configuration fetching #20

Closed SKalt closed 5 years ago

SKalt commented 6 years ago

the problem

Currently, docblock-python does all the heavy lifting: given a cursor position, it parses the class/function declaration around it, gets the docstring style configuration, and turns all of that into a well-formatted docstring. Parsing the code in particular is prone to errors (example below) and duplicates work already done by other python tools.

the benefits of decoupling

Decoupling the parsing, formatting, and configuration-getting abilities of the module would make it easier to integrate with code parsing services in the future. Writing more functional code would also make writing unit tests easier, and might even allow code reuse in other projects (I'd love to be able to import {googleStyleDocstring, getFunctionSignature} from 'docblock-python').

I've taken a first stab at making the codebase more functional in #21 .

Examples of code parsing services
  • The language-python and MagicPython grammars highlight function and class signatures by marking the atom HTML with scopes like .meta.function.parameters.python, .variable.parameter.function.python.
  • the python language server provided by ide-python could be queried via a [textDocument/signatureHelp request](https://microsoft.github.io/language-server-protocol/specification#signature-help-request-leftwards_arrow_with_hook). Example:
    {
      "signatures": [
        {
          "label": "foo(a: str=1) ->int",
          "documentation": "",
          "parameters": [
            {
              "label": "a",
              "documentation": null
            }
          ]
        }
      ],
      "activeSignature": 0,
      "activeParameter": 0
    }
    
Example python parsing bug Example of the kind of parsing error parsing code is liable to:
async       def      foo(): return 1
won't be recognized since it doesn't have the exact string `async def` in it.
def bar(a: Complex[[TypeHint], int] = someOtherFunctionAsADefault(...)[::-1], ) -> int: ...
spadarian commented 6 years ago

I agree with that some refactoring is needed to make the code more functional.

I also like the idea of re-using some code but, actually, I don't mind that the plugin does most of the lifting to avoid installing extra things just to use 1 function. language-python probably is installed if you are writing Python code, so we could have a look at that.

About the parsing bugs, I get the correct docstring for your first example:

async       def      foo(): return 1
"""Short summary.

Returns
-------
type
    Description of returned object.

"""

For the second one... I guess it is not difficult to fix (I generally don't use trailing commas).

SKalt commented 6 years ago

I can certainly respect keeping this zero-dependency. I'll focus my efforts on breaking things finding edge cases, writing unit and integration tests, and doing more functional refactoring.

spadarian commented 6 years ago

Thanks. I appreciate your interest and help.

I'm a bit busy these days, but I'll try to review your PR (#21) tomorrow or the day after.

SKalt commented 6 years ago

No rush! This project has become an essential part of my workflow, so I'm happy to give back.

On Tue, Sep 25, 2018 at 9:29 AM José Padarian notifications@github.com wrote:

Thanks. I appreciate your interest and help.

I'm a bit busy these days, but I'll try to review your PR (#21 https://github.com/spadarian/docblock-python/pull/21) tomorrow or the day after.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spadarian/docblock-python/issues/20#issuecomment-424343495, or mute the thread https://github.com/notifications/unsubscribe-auth/AJ9G5avandQnb7V8gUrAB9r6MFVoaBb6ks5uei_JgaJpZM4W1vjm .

spadarian commented 5 years ago

We can re-open this if you have more suggestions @SKalt

SKalt commented 5 years ago

Thanks for merging this (and killing all those issues)! I don't have anything in mind.