Skip to content

Conversation

@jspaezp
Copy link
Contributor

@jspaezp jspaezp commented Sep 12, 2022

Improved the toml parsing so it supports BooleanOptionalAction arguments in it.
(also added tests for it ...)

[tool.pydocstringformatter]
write = false
style = ["numpydoc"]
strip-whitespaces = true  # <<<<<- previously unsupported
split-summary-body = false # <<<<<- previously unsupported
no-numpydoc-section-hyphen-length = true # <<<<<- previously unsupported

.... where should I document this?

@github-actions

This comment has been minimized.

Copy link
Collaborator

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for adding this !

@Pierre-Sassoulas Pierre-Sassoulas added the enhancement New feature or request label Sep 12, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 0.7.3 milestone Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #166 (0242e5e) into main (9268fd6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #166   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          591       604   +13     
=========================================
+ Hits           591       604   +13     
Impacted Files Coverage Δ
...ydocstringformatter/_configuration/toml_parsing.py 100.00% <100.00%> (ø)

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 12, 2022

@Pierre-Sassoulas I think I should also add this to the documentation on the toml configuration, do you happen to know where that should be added?

@Pierre-Sassoulas
Copy link
Collaborator

This is the full doc on configuration: https://pydocstringformatter.readthedocs.io/en/latest/?badge=latest#configuration

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 12, 2022

Oh i see... so that is getting auto generated from the readme ...

Should we modify:

[tool.pydocstringformatter]
write = true
exclude = "**/my_dir/**,**/my_other_dir/**"
# Or:
exclude = ["**/my_dir/**", "**/my_other_dir/**"]

To this:

[tool.pydocstringformatter]
write = true
exclude = "**/my_dir/**,**/my_other_dir/**"
# Or:
exclude = ["**/my_dir/**", "**/my_other_dir/**"]
style = ["numpydoc"]
strip-whitespaces = true
split-summary-body = false
no-numpydoc-section-hyphen-length = true

to exemplify how to set other argument types.
LMK what you think

@Pierre-Sassoulas
Copy link
Collaborator

Maybe we could autogenerate the full list of possible options with default values ?

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 13, 2022

I like that idea but it would not exemplify the use of 'no-xxxxx = true' style configs. ... Maybe also have comments showing the equivalent config on the negative form ??

This could be a feature for a later improvement in documentation (and probably not inside the readme ... So it does not get too clutered)

@Pierre-Sassoulas
Copy link
Collaborator

I think the no-x style is only used in CLI (or should) this is given by the --help command so we don't need to document it imo'

@DanielNoord
Copy link
Owner

The no- options are needed to disable default checks. I think they should indeed be shown as a comment in the example config.

For pylint we use tomlkit. This is one of the few parsers for toml that saves comments and style.
A solution where we initialise all options and then write a file with tomlkit would be nice.
Perhaps it would be even better if we could create a --create-config command so that we can also offer it as a cli
tool?

@Pierre-Sassoulas
Copy link
Collaborator

The no- options are needed to disable default checks

Isn't it possible to do x=false or no-x=true ?

@DanielNoord
Copy link
Owner

The no- options are needed to disable default checks

Isn't it possible to do x=false or no-x=true ?

It depends on how we handle that in this PR. We could just disallow them completely as well?

What do you think makes most sense @jspaezp?

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 14, 2022

Hello there!

I thought about it for a while...
and I think the most consistent behavior would be to allow "--no" version. I really like the idea of the config being as close as possible to copy/pasting the command line arguments.

regarding: #166 (comment)

Isn't it possible to do x=false or no-x=true ?

its easy but not SUUUUPER EASY, just having to consider it robust to arguments that in the future might start with --no- but are not of the same "optional boolean" type.

I will add the mods to the readme in the meantime

@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Collaborator

I meant it's possible to do it as a user (so you should always be able to use simply 'x' which is simpler both for the user and for us). Imo we should not permit to use no-x in configuration, there's very little benefit and a lot of headaches to expect.

@github-actions

This comment has been minimized.

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 14, 2022

Alright, I added the enforcement of the positive form of the arguments (+testing, +updated readme, +informative error message).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I like the clear error message ! LGTM, I'll let Daniel merge.

Co-authored-by: Daniël van Noord <[email protected]>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jspaezp jspaezp requested a review from DanielNoord September 14, 2022 10:03
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@DanielNoord
Copy link
Owner

Thanks once again @jspaezp 🎉 😄

@DanielNoord DanielNoord enabled auto-merge (squash) September 14, 2022 11:01
@github-actions
Copy link

According to the primer, this change has no effect on the checked open source code. 🤖🎉

@DanielNoord DanielNoord merged commit c48a672 into DanielNoord:main Sep 14, 2022
@jspaezp jspaezp deleted the feature/improved_toml_support branch September 14, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants