-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
Added class and module docstrings #174
base: master
Are you sure you want to change the base?
Conversation
…tring into feature/module_docstring
…tern allows use of getFunctionName() method; parseParameters: parseExceptions: [] inside docstringType === class
Feature/module docstring
updated changelog; cleaning up code
refactoring for code climate compliance
still simplifying get_definition
refatoring...
one more try
Hi this pr looks fantastic! I will review it this weekend. |
@NilsJPWerner any updates on your side? |
Nice work this would be awesome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay. I finally found time to check your pr. It looks very good on inspection. There are a few things that need cleaning up and adding, and I am going to try it out for a few days to check that there are no regressions.
@@ -0,0 +1,75 @@ | |||
"use strict"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this? Were the existing integration tests not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually sure. I did not do that; perhaps my colleague who did a little work did. I will find out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that was me, i'm not sure why i put it there. if the tests run without it, then remove it. i would imagine it was me being nit-picky bc my extension was telling me to add it or something
@@ -0,0 +1,18 @@ | |||
export function getDocstringType(functionDefinition: string, linePosition: number): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should turn the return type into a string literal type so that we restrict to only types we define.
} | ||
|
||
else { | ||
return "method"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be renamed to function since not all functions are methods.
returns: undefined, | ||
yields: undefined, | ||
exceptions: [], | ||
classes: parseMethods(body, /(?:class)\s/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be split into 2 functions instead of passing a regex into it
} | ||
let var_ = match[1]; | ||
let type_ = guessType(match[1]); | ||
if (!containsAttribute(attributes, var_) && !containsAttribute(args, var_) && !containsAttribute(kwargs, var_)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by this? What is going on here?
class TestClass(object): | ||
"""[summary] | ||
|
||
:param arg1: [description] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need an integration test for attributes.
@tracetidwell thank you for contributing to this project. This feature would help out so many of us that have expressed interest in issue #27. It would be really helpful if you addressed the comments that the repo owner @NilsJPWerner has requested, then he can followup and get this merged and published for us all to use :) |
After way, WAY too long, I am going to finish this. I resolved a number of issues, left comments on a couple others, and need to work on the rest. If this is not being maintained or doesn't seem worth it, please let me know. |
I haven't touched this repo in years, but I think I still have the repo cloned locally. @tracetidwell let me know if you need something from me. would love to help get this completed and merged. |
Any news on this? |
Bumping this again? Would love to see this merged? If not, does anyone know any alternatives? |
If @tracetidwell or someone else wants to pick up the PR again or start fresh I would be happy to review and merge. Right now I am focused on adding the ability to update docstrings, and so work on module and class docstrings will have to come afterwards. |
WANT! |
This would be super nice, someone finalize this and add it |
Yes, please make this happen 🙏 |
Really, sitting in this state for over two years? I don't think this is being maintained any longer. |
Need this please, ASAP! |
Any updates on this? |
Did anyone perhaps forked and added this feature? I would love to use it! |
This still seems like useful functionality. Is there any chance of it being introduced? What about alternatives? |
@NilsJPWerner @PhilipNelson5 @tracetidwell Any movement on this? This is a really cool tool and this PR would be a great addition. If this tool as a whole is no longer being maintained then fair enough :) |
@tracetidwell @NilsJPWerner any upate on this? |
class docstrings are created under the class definition. They read parameters from the init() method and scan the entire class for attributes. Both parameters and attributes are listed.
module docstrings are created at the first line of the script. They scan the entire file and list out all top-level classes and methods defined therein.
All tests are passing, and I added a few unit tests and integration tests. I am not an expert at TypeScript, so this might can be improved, but I've been using this on my code for a few months now with no problems.