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

support for google parameters extras (typehint, optional/required) #3

Closed
jmatzdorff-cpi opened this issue Jan 22, 2019 · 8 comments
Closed
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@jmatzdorff-cpi
Copy link
Contributor

Expected Behavior

Want to be able to have args such as:

Args:
            no_extra: no extra parameters
            one_extra(one): one extra parameter contained in the ()
            multi_extra(one, two, three): three extra parameters contained in the ()

            no_extra: no extra parameters, but this line does span
                multiple lines, so there's that
            one_extra(one): one extra parameter contained in the (), but this line does span
                multiple lines, so there's that
            multi_extra(one, two, three): three extra parameters contained in the (), but this line does span
                multiple lines, so there's that

Which should be supported by google-style docs:

https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

Actual Behavior

you can only have the variable name before the ":", no extra descriptions before (in parens). if you do have some, the args will get unformatted.

you can do it after the ":" of course -- but that's an ugly workaround (IMO) and doesn't follow spec.

Steps to Reproduce

see expected vs actual :)

Additional info

  • pdoc version: 0.5.1

This is just a regex update; in google (def google) in the file html_helpers, the lamda-regex should be:

               _googledoc_sections=partial(
                   re.compile(r'(?<=\n\n)(\w+):$\n((?:\n?(?: {4}.*|$))+)', re.MULTILINE).sub,
                   lambda m, _params=partial(
                           re.compile(r'^([\w*]+\s*(?:\(.*\))?)(?: \(([\w. ]+)\))?: '
                                      r'((?:.*)(?:\n(?: {4}.*|$))*)', re.MULTILINE).sub,
                           lambda m: _ToMarkdown._deflist(*m.groups())): (
                       '\n{}\n-----\n{}'.format(
                           m.group(1), _params(inspect.cleandoc(m.group(2))))))):

though i haven't completely tested to see if this works (worked for the above expected behavior examples) in every possible situation. i am having a slight issue getting the test to run.

@kernc
Copy link
Member

kernc commented Jan 22, 2019

The first line of the regex already accounts for parenthesized types (not arbitrary parameters):

re.compile(r'^([\w*]+)(?: \(([\w. ]+)\))?: '

See the test example: https://pdoc3.github.io/pdoc/doc/pdoc/test/example_pkg/index.html#pdoc.test.example_pkg.google

You make your regex accept .* in parentheses, but I think simply adding comma to the list of accepted characters (:+1:) would work for your case:

re.compile(r'^([\w*]+)(?: \(([\w., ]+)\))?: '

Since docstring conversion is applied automatically, we limit the possibility for false positives if we don't make it match .*. Do you feel you need any other characters besides comma, and why?

Additionally, in your example you have no space between the argument name and parenthesized parameter. A regex like this (:+1:) would support that as well.

re.compile(r'^([\w*]+)(?: *\(([\w., ]+)\))?: '

@kernc
Copy link
Member

kernc commented Jan 22, 2019

Would you like to make a pull request for that?

i am having a slight issue getting the test to run.

What issue are you having?

@jmatzdorff-cpi
Copy link
Contributor Author

ah, you are correct ... my code started out with using comma character and didn't work and so i went to fixing that before i even tried testing with just a single type, which led me to thinking it didn't work at all. apologies.

no i can't imagine needing any other character other than comma (and even that i suppose is debatable, but i think google style is supposed to support that).

adding the comma to the regex works great. i also didn't realize it converts it to numpy-style on the output (rather than keeping the original formatting, such as "var (int): description"). jumped too many guns there.

i'll submit a PR with a test here in a bit.

i was being a bit cheekish with my comment about getting tests to run. my issue was that i didn't want to take the time to do it ;) -- but i have them up and running now.

@kernc kernc added this to the 0.5.2 milestone Jan 24, 2019
@kernc
Copy link
Member

kernc commented Jan 27, 2019

Still interested? Need any help? 😃

@kernc kernc added bug Something isn't working good first issue Good for newcomers labels Jan 27, 2019
@jmatzdorff-cpi
Copy link
Contributor Author

i tried pushing changes upstream to a branch that i created - so as to make a PR. but i don't seem to have permissions to do that to your repo. So not sure how to do any changes.

i updated as you mentioned (also added in a "=" sign character because i find it helpful to have in the doc string "default=" as well) and updated the tests. but not sure how i can push that up.

@kernc
Copy link
Member

kernc commented Jan 28, 2019

Yes, you can't just push to this repo. First you fork and make your own repo and push to a branch on it. Then, GitHub will permit (and recommend) you to do a pull request: https://gist.github.com/Chaser324/ce0505fbed06b947d962

@kernc
Copy link
Member

kernc commented Jan 30, 2019

@jmatzdorff-cpi Happy for you to finish this. 👍

You can add your own remote to your checkout with:

# Rename the current remote "origin" (that points to pdoc3/pdoc) to "upstream"
git remote rename origin upstream

# Add new "origin" remote
git remote add origin git@github.com:jmatzdorff-cpi/pdoc.git

# Push your changes to your new "origin" remote 
# regardless of what the branch tracking is set to
git push origin HEAD:fix-3
# Note, this will create a new remote feature branch "fix3".

Afterwards when you go to pdoc3/pdoc, GitHub UI should propose a PR from the newly-pushed branch.

@jmatzdorff-cpi
Copy link
Contributor Author

well, when you give me exact steps; i can't exactly say i don't what to do :)

anyhow, it's pushed and PR requested!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Development

No branches or pull requests

2 participants