bandit icon indicating copy to clipboard operation
bandit copied to clipboard

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

Open bittner opened this issue 3 years ago • 9 comments

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 avatar Apr 03 '22 17:04 bittner

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

bittner avatar Apr 05 '22 15:04 bittner

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.

TreeKat71 avatar Apr 08 '22 05:04 TreeKat71

Please, review and merge!

bittner avatar Apr 11 '22 14:04 bittner

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

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

bittner avatar Apr 25 '22 10:04 bittner

@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?

bittner avatar Aug 03 '22 09:08 bittner

@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 avatar Aug 03 '22 09:08 gdalmau

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

bittner avatar Aug 03 '22 12:08 bittner

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 avatar Aug 03 '22 18:08 bittner

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?

bittner avatar Aug 07 '22 10:08 bittner

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)

sigmavirus24 avatar Aug 11 '22 08:08 sigmavirus24

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 avatar Aug 11 '22 10:08 bittner

Thanks for merging! :+1:

bittner avatar Aug 11 '22 17:08 bittner