[docs] Mention `exclude_dirs` option available in TOML and YAML #876

bittner · 2022-04-03T17:30:38Z

As discovered in #488 (latest comments) and reported in #528.

Also attempts to improve the documentation code by aligning code block indenting replacing simple code blocks (::) with source code markup code blocks in reStructuredText.

Side Notes

Despite the name, exclude_dirs also recognizes single files, not only directories.
In the long run it may make sense to consolidate the two options exclude and exclude_dirs.

bittner · 2022-04-05T15:02:05Z

Can we merge these changes, so developers can have it easier finding information about how to configure Bandit?

TreeKat71 · 2022-04-08T05:55:33Z

This is what I am looking for and it is the missing part in the document.
exclude is not working in the pyproject.toml
I think it does help developer can find information easier.

bittner · 2022-04-11T14:24:45Z

Please, review and merge!

bittner · 2022-04-25T10:43:05Z

Rebased the PR to resolve conflicts after #868 was merged, yesterday.

@ericwb Can you review and merge this PR, please?

bittner · 2022-08-03T09:37:27Z

@gdalmau Thanks for your approval! I rebased the PR and resolved the conflict that had been introduced since I opened it. Can you approve again, please? And maybe merge?

gdalmau · 2022-08-03T09:53:16Z

@bittner Sorry I'm not a member of PyCQA nor bandit. I just checked the changes and they looked good to me so I approved if that means anything to the members of this project :)

I will recheck the changes and approve (or not) so hopefully we can see this PR merged.

gdalmau · 2022-08-03T10:03:36Z

doc/source/start.rst

+If you want to include TOML support, install it with the `toml` extras:
+
+.. code-block:: console
+
+    pip install bandit[toml]


I just noticed that tomli is required if you use a pyproject.toml. In my case our CI didn't break because it was installed via other packages, but a bare install makes it indeed not work.

Since the setup.cfg file seems to also have an extra for yaml files:

bandit/setup.cfg

Lines 30 to 34 in caae4ee

[extras]

yaml =

PyYAML

toml =

tomli>=1.1.0; python_version < "3.11"

Should we mention it as well in here? Or does a .bandit YAML file work with just installing pip install bandit?

Hmm, I'm confused. 🤔 Installing the toml extra, that's what the pip install bandit[toml] does. Anything else needed from your point of view in addition to those instructions?

What I mean is that in order to make bandit process a pyproject.toml file it needs to be installed via pip install bandit[toml]. Does it need to be installed via pip install bandit[yaml] for yaml files? If so, it should probably be in this docs as well.

I understand! So, "If you want to include TOML support" is not clear enough?

Argh, YAML! Now I understand. Let me verify this locally.

I can confirm, the latest version of PyYAML (not the one in requirements.txt) is installed as a dependency when you install Bandit via pip install bandit.

I'm a bit puzzled: I absolutely can't see in the source code where the dependencies being installed are defined.

bittner · 2022-08-03T12:11:27Z

@ericwb @lukehinds @sigmavirus24 Could you approve and merge, please?

sigmavirus24

@bittner there's way more here than just mentioning that option. You've reformatted large swaths of text and many of these things we simply won't accept in what should be a relatively small patch.

Case-in-point: pre-commit formats the hooks in exactly the way we have them formatted. If you look at it's documentation, all hooks are written exactly as we wrote ours.

bittner · 2022-08-03T18:41:33Z

You've reformatted large swaths of text and many of these things we simply won't accept in what should be a relatively small patch.

Please note that the PR has 2 separate commits, dedicated to changes with their own scope.

With respect to the formatting, I attempt to modernize the reStructuredText syntax, with a uniform use of .. code-block:: and non-inlined hyperlinks. Isn't this fair to be included in what is a change to the documentation?

Case-in-point: pre-commit formats the hooks in exactly the way we have them formatted. If you look at it's documentation, all hooks are written exactly as we wrote ours.

Fair point. I didn't know. I'd have to adapt the pre-commit hook configuration with those changes, fair enough. That would certainly warrant a dedicated PR,—Let me revert those changes. I apologize.

bittner · 2022-08-07T10:16:36Z

I reverted the changes to the displayed source code that apparently gets reformatted by the pre-commit hook, when installed.

The PR description now mentions the reformatting of the documentation code (which affects the code portions modified in this PR and is hence difficult to contribute independently in parallel). I hope you see the value in the change. If there's anything you want adapted in addition, please let me know.

@ericwb @lukehinds @sigmavirus24 Could you approve and merge now, please?

doc/source/config.rst

sigmavirus24 · 2022-08-11T08:55:41Z

With respect to the formatting, I attempt to modernize the reStructuredText syntax, with a uniform use of .. code-block:: and non-inlined hyperlinks. Isn't this fair to be included in what is a change to the documentation

The project has a history of separating these out. A switch to code-blocks should be a fast merge but there are also formatting changes to the docs last I checked with line wrapping, etc.

Non-inlined URLs are my preference and again standardizing s hour be easy to review. Finally, this change which is important should have been easy to merge but was allowed down by the additional noise making it harder to review with confidence something else wasn't missed. It's a matter of opinion but every comment you've left recently had been instructing people to merge it. That might have happened years ago if there had been less noise making the important change (documenting this option) easier to see and comment on with the shit tools available to the maintainers (because separate commits do not in fact improve the review experience on GitHub)

Make configuration examples easier to understand and use Explain how to install TOML support As discovered in #488 and reported in #528.

bittner · 2022-08-11T10:29:19Z

Ian, thanks for taking the time to comment. I think the situation has now improved.

The line wrapping has been undone
Hyperlinks are non-inline, following your preference
Code blocks provide syntax highlighting, which improves the reading experience with different file types (INI, YAML, TOML)

The documentation benefits from a nicer reading experience, and the documentation code has been modernized with related changes. While I second your concerns of the review experience, the benefits hopefully speak in favor of this larger-than-bare-minimum PR.

bittner · 2022-08-11T17:34:43Z

Thanks for merging! 👍

Move the list of excluded files from --exclude in tox.ini to exclude_dirs in pyproject.toml to centralize configuration in pyproject.toml and make it accessible to tools and bandit invocations outside of tox. - Remove the comment that exclude is ignored by bandit 1.6.3+, which was fixed by PyCQA/bandit#722 in bandit 1.7.1. - Change exclude (which only works for INI files) to exclude_dirs (which only works for TOML and YAML files), as described in PyCQA/bandit#876 - Add /.git/ and /__pycache__/ to exclude_dirs to match --exclude. - Remove --exclude from invocation in tox.ini Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

bittner requested review from ericwb, lukehinds, ghugo and sigmavirus24 as code owners April 3, 2022 17:30

gdalmau approved these changes Aug 2, 2022

View reviewed changes

gdalmau approved these changes Aug 3, 2022

View reviewed changes

gdalmau reviewed Aug 3, 2022

View reviewed changes

sigmavirus24 requested changes Aug 3, 2022

View reviewed changes

ericwb reviewed Aug 8, 2022

View reviewed changes

doc/source/config.rst Outdated Show resolved Hide resolved

bittner requested review from sigmavirus24 and ericwb and removed request for ghugo August 9, 2022 13:50

bittner added 2 commits August 11, 2022 11:32

[docs] Use code-blocks for syntax highighting, un-inline hyperlinks

3cbac44

[docs] Mention exclude_dirs option available in TOML and YAML

0a2c12b

Make configuration examples easier to understand and use Explain how to install TOML support As discovered in #488 and reported in #528.

sigmavirus24 enabled auto-merge (squash) August 11, 2022 16:13

sigmavirus24 approved these changes Aug 11, 2022

View reviewed changes

sigmavirus24 merged commit ebd168a into PyCQA:main Aug 11, 2022

bittner deleted the docs/explain-exclude-dirs-option branch August 11, 2022 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Mention `exclude_dirs` option available in TOML and YAML #876

[docs] Mention `exclude_dirs` option available in TOML and YAML #876

bittner commented Apr 3, 2022 •

edited

bittner commented Apr 5, 2022

TreeKat71 commented Apr 8, 2022 •

edited

bittner commented Apr 11, 2022

bittner commented Apr 25, 2022

bittner commented Aug 3, 2022 •

edited

gdalmau commented Aug 3, 2022

gdalmau Aug 3, 2022

bittner Aug 3, 2022

gdalmau Aug 3, 2022

bittner Aug 3, 2022

bittner Aug 3, 2022

bittner Aug 7, 2022

bittner commented Aug 3, 2022

sigmavirus24 left a comment

bittner commented Aug 3, 2022 •

edited

bittner commented Aug 7, 2022 •

edited

sigmavirus24 commented Aug 11, 2022

bittner commented Aug 11, 2022

bittner commented Aug 11, 2022

	[extras]
	yaml =
	PyYAML
	toml =
	tomli>=1.1.0; python_version < "3.11"

[docs] Mention exclude_dirs option available in TOML and YAML #876

[docs] Mention exclude_dirs option available in TOML and YAML #876

Conversation

bittner commented Apr 3, 2022 • edited

Side Notes

bittner commented Apr 5, 2022

TreeKat71 commented Apr 8, 2022 • edited

bittner commented Apr 11, 2022

bittner commented Apr 25, 2022

bittner commented Aug 3, 2022 • edited

gdalmau commented Aug 3, 2022

gdalmau Aug 3, 2022

Choose a reason for hiding this comment

bittner Aug 3, 2022

Choose a reason for hiding this comment

gdalmau Aug 3, 2022

Choose a reason for hiding this comment

bittner Aug 3, 2022

Choose a reason for hiding this comment

bittner Aug 3, 2022

Choose a reason for hiding this comment

bittner Aug 7, 2022

Choose a reason for hiding this comment

bittner commented Aug 3, 2022

sigmavirus24 left a comment

Choose a reason for hiding this comment

bittner commented Aug 3, 2022 • edited

bittner commented Aug 7, 2022 • edited

sigmavirus24 commented Aug 11, 2022

bittner commented Aug 11, 2022

bittner commented Aug 11, 2022

[docs] Mention `exclude_dirs` option available in TOML and YAML #876

[docs] Mention `exclude_dirs` option available in TOML and YAML #876

bittner commented Apr 3, 2022 •

edited

TreeKat71 commented Apr 8, 2022 •

edited

bittner commented Aug 3, 2022 •

edited

bittner commented Aug 3, 2022 •

edited

bittner commented Aug 7, 2022 •

edited