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

Handling of *args and **kwargs #48

Closed
Laleee opened this issue Dec 16, 2022 · 7 comments · Fixed by #49
Closed

Handling of *args and **kwargs #48

Laleee opened this issue Dec 16, 2022 · 7 comments · Fixed by #49
Assignees

Comments

@Laleee
Copy link

Laleee commented Dec 16, 2022

Hello,

I'm not sure what's the convention of documenting *args and **kwargs but I think they are usually skipped.

What do you think about ignoring them (if they are not present in the docstring)?

@jshwi
Copy link
Owner

jshwi commented Dec 17, 2022

Hi @Laleee,

thanks again for reaching out

For me they get documented like any other parameter and are just as important

There is a not one consensus on how to document **kwargs, as their types and uses can vary
For this, **kwargs can be documented as the bundle (kwargs/**kwargs) or as each individual keyword (key/keyword) depending on their use
Due to the way this package handles kwargs, being the standard for documenting them is a little more of a grey area, it makes sense you might not see a reason to document them as a param, or at all

*args are easier to just document as args or *args for the value and type that there may be several of

There could easily be a switch, such as --ignore-kwargs and ignore-args which wouldn't effect the program at all for those that don't want to pass that arg

I see more of a use case for ignoring kwargs, but I can easily implement this feature for args while I'm at it

I'll see what I can do, and thanks again!

@jshwi jshwi self-assigned this Dec 17, 2022
@Laleee
Copy link
Author

Laleee commented Dec 18, 2022

Adding ignore arguments seems like a good idea

FYI: This formatting is also legitimate: mkdocstrings/pytkdocs@0133369

I'm not sure if it's supported but I just wanted to give you a head's up.

@jshwi
Copy link
Owner

jshwi commented Dec 20, 2022

Hi @Laleee,
I've worked on the implementation, I'll push it soon after some tests
At the moment, with Sphinx/RST at least, args and kwargs are a little more ambiguous in the docstring than the signature
The implementation would likely make checks that passed originally, with documented args and kwargs, fail - could be potentially confusing. What are your thoughts on this?
Either way, I'll push this soon and you can try it out
Cheers,
Stephen

jshwi added a commit that referenced this issue Dec 20, 2022
jshwi added a commit that referenced this issue Dec 20, 2022
@jshwi jshwi linked a pull request Dec 20, 2022 that will close this issue
@jshwi jshwi closed this as completed in #49 Dec 21, 2022
@Laleee
Copy link
Author

Laleee commented Dec 23, 2022

Hello @jshwi,

Trying to test this I found a bug related to the pre-commit configuration.

pre-commit-config.yaml

-   repo: https://github.com/jshwi/docsig
    rev: v0.33.0
    hooks:
      - id: docsig
        name: docsig
        entry: docsig
        language: system
        types: [python]
        require_serial: true
        args:
          - "-i"
          - "--ignore-kwargs"

I get an error:
docsig: error: unrecognized arguments: --ignore-kwargs

Then I tried manually and the version wasn't the latest. After the manual update (pip install -U docsig), both manual and pre-commit worked.

This shouldn't be the case since I've specified the exact version in the pre-commit (I guess it should have it's own environment based on the yaml specification).

Can you check if this is due to bad integration?

Regarding the feature itself, everything seems to be working as expected so far. 🎉

Best,
Lazar

@jshwi
Copy link
Owner

jshwi commented Dec 24, 2022

yup @Laleee, definitely something wrong there, bear with me, I'll see what's going on

@jshwi
Copy link
Owner

jshwi commented Dec 24, 2022

Opened #50 for tracking

@jshwi
Copy link
Owner

jshwi commented Dec 24, 2022

Hi @Laleee,

Almost went down the rabbit hole with #50

I thought it was an issue with the hooks file, turns out it was an issue with the documentation

I don't currently us pre-commit, I find myself rebasing a fair bit instead
I'm more compelled to now though, as I learn more about it

The key language: system is the problem
it will use your system PATH for the executable, and not the isolated pre-commit environment (it won't even build the revision specified) so it's up to the user in this case to have the executable installed
That's why it was using an outdated version, if you didn't have the package installed at all it would have raised Executable docsig not found

By using language: python, this implicitly instructs pre-commit to use the build tool necessary, in this case
pip install .
and so it will build the revision cloned

I sort of borrowed the format from pylint, which was the worst one I could have used, as it HAS to use your virtualenv installation to import the package to lint

I've updated this documentation

Thanks for spotting this one!
Cheers

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 a pull request may close this issue.

2 participants