-
-
Notifications
You must be signed in to change notification settings - Fork 618
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
PEP-518 support: configure bandit via pyproject.toml #401
Conversation
|
||
|
||
class TestTomlConfig(TestConfigCompat): | ||
sample = textwrap.dedent(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block would be good as a fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't fixture. I want to run same test cases twice for two different configs. Fixture, contrariwise, allows you to reuse same set ups in different test cases. Inheritance works perfect in this case.
2 from 3 review points applied. Thank you for good review :) |
@ericwb, what about this PR? Does you have any other requests? |
@ericwb, could you review it again, please? It's ready to merge, I hope. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how this is implemented using the same interface as YAML config 👍
Thank you ❤️ Could you, please, poke the code owner to finally merge it? |
@orsinium sadly I don't have any more poking prowess than you, but what I can say that I'm also interested in this features, especially as it possibly may (or may not) affect #318 . That said this eature seems to stand on its own and is worth merging IMO. you will have to update the PR as there already some file conflicts @ericwb do you think its ready for merging? (after resolving conflicts) |
I'm reluctant to add another way to process configuration which adds a new run-time dependency (toml). |
Why? As you said, this is runtime dependency. Nothing changes for users who prefer old format. But support of PEP-518 will be very helpful for most of other people. You already have at least two issues about it. |
The requirements "issue" can be mitigated by making toml "extra" requirement (https://docs.openstack.org/pbr/3.1.0/index.html#extra-requirements) , and changing the code that on failed toml import it just doesn't attempt to read pyproject.toml |
|
Personally, I'd prefer to include a There is a |
@bittner I would really appreciate this being merged and helping set the pace for other projects, as it's frustrating that so many projects are just sitting with their pyproject work in a PR holding pattern. EDIT: Maybe I'm wrong, based on the discussion on the PR in the Flake8 repository, we who thought pyproject.toml was for all tools may have misunderstood the PEP's intention. Now I can understand where the holding comes from, because it's unclear if we should move forward with this or not. |
I fear my comment from above is unclear: I'm not against a
... when you find the first match then stop searching and use that configuration. P.S.: I find myself in trouble trying to find the documentation on which files Bandit looks for for the INI-style configuration. I know it was somewhere. I only find the config chapter, but that's on the YAML. The docs really need an overhaul... 😟 (see issue #396) |
@bittner, my PR doesn't affect config lookup at all. In this realization you have to explicitly specify path to the pyproject.toml config to read it. Config lookup can be changed in a separated PR after (and if) this will be merged |
So .. is this never getting merged? |
Closes #401 |
Hi, also looking forward for this PR to get merged. |
@orsinium @rooterkyberian @ericwb @lukehinds What's happening with this PR? Anything I can do? |
Nothing's happening from my side. I suppose it only awaits merging. The only thing I can do is to keep the branch up-to-date with the target branch, I guess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK with this, but will defer to @ericwb who had some comments in last review.
hello, any updates here? It would be great to see that feature in official release. |
Would love to see this PR being merged as well |
Yeah, same here, looks good? |
It doesn't look good as there are changes requested by a maintainer. |
From what I understand, @ericwb requested to change a block into a fixture and @orsinium answered, why the current solution is fine. That blocked the merge and it was not resolved. Later @ericwb said he was reluctant about the merge, because another dependency would be added, which ended in a new commit to improve this handling. My questions would be if the fixture and dependency topics are still blocking topics @ericwb? What can be done to resolve them? We would really love to see this feature being added to improve our configuration setup, because bandit is currently the only one in our stack not supporting pyproject.toml |
Co-authored-by: Lionel Bersee <lionel1232@gmail.com>
Co-authored-by: Lionel Bersee <lionel1232@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small change requested and we can merge this. Thanks for your patience.
Co-authored-by: Eric Brown <ericwb@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you for cool lib.
Added support for configuring via pyproject.toml (
tool.bandit
section). Closes #212, because now all projects have moved from setup.cfg to pyproject.toml..toml
as toml and get thetool.bandit
section.Close #550 as well