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

Break out CLI support from the main library #47

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Break out CLI support from the main library #47

merged 3 commits into from
Nov 27, 2023

Conversation

0xDEC0DE
Copy link
Collaborator

@0xDEC0DE 0xDEC0DE commented Nov 27, 2023

Move the CLI tool into a cli.py, so that consumers can import the
library without needing to pull in Click, etc.

Also move the CLI-specific requirements into an extras section, and
regenerate/bump requirements

Fixes: Issue #20


This change is Reviewable

Ruff implements all of the Bandit rules we care about, is faster,
doesn't have as many idiosyncrasies, and appears to be more actively
maintained.

Activate the Bandit tests when we run Ruff, and throw Bandit down the
stairs.

Of note: Ruff does not complain about insecure modules being imported,
only about them being used (unused imports are a separate error)
This means far fewer annotations are required for things like `pickle`
Copy link
Collaborator

@bigjools bigjools left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @0xDEC0DE and @juledwar)


requirements/requirements.txt line 321 at r1 (raw file):

    --hash=sha256:4a7317d5e3b17a3dccb6a8cfe67dab65b20551404c52c8ed41279fa4f0cb4cda \
    --hash=sha256:d1377122a5a00e2f940ee482999518efe16d745d423a670c27773dfbc3c9a7d9
    # via stevedore

Grrr. dogpile-cache pulling in stuff that uses PBR.


soufi/cli.py line 328 at r1 (raw file):

if __name__ == "__main__":
    main()

Did you do a git move on this file? I can't remember if it tracks those properly or not, and we lose the blame history.


soufi/finders/alpine.py line 249 at r1 (raw file):

        tmp_file_name = pathlib.Path(temp_dir) / name
        with closing(
            # B310 restricts permitted schemes, but we only call with ftp here.

S310 now


tools/compile-requirements line 43 at r1 (raw file):

compile ${REQ_DIR}/requirements-test.txt --extra test
compile ${REQ_DIR}/requirements-bootstrap.txt --extra bootstrap
compile ${REQ_DIR}/requirements-cli.txt --extra cli

You need to update the README since it references the CLI and the installation instruction is now incorrect.

@0xDEC0DE
Copy link
Collaborator Author

soufi/cli.py line 328 at r1 (raw file):

Previously, bigjools (Julian Edwards) wrote…

Did you do a git move on this file? I can't remember if it tracks those properly or not, and we lose the blame history.

I did, but I also dropped in an empty __init__.py, which apparently confuses and frightens git.

@0xDEC0DE
Copy link
Collaborator Author

soufi/cli.py line 328 at r1 (raw file):

Previously, 0xDEC0DE (Nicolas Simonds) wrote…

I did, but I also dropped in an empty __init__.py, which apparently confuses and frightens git.

I suppose the obvious tacky workaround is to move in one commit, and add in another, and accept that there will be a commit in the stream that "doesn't work"

@0xDEC0DE
Copy link
Collaborator Author

soufi/finders/alpine.py line 249 at r1 (raw file):

Previously, bigjools (Julian Edwards) wrote…

S310 now

Whoops

@0xDEC0DE
Copy link
Collaborator Author

tools/compile-requirements line 43 at r1 (raw file):

Previously, bigjools (Julian Edwards) wrote…

You need to update the README since it references the CLI and the installation instruction is now incorrect.

Whoopsie!

Move the CLI tool into a `cli.py`, so that consumers can import the
library without needing to pull in Click, etc.

Also move the CLI-specific requirements into an extras section, and
regenerate/bump requirements

Fixes: Issue #20
Copy link
Collaborator Author

@0xDEC0DE 0xDEC0DE left a comment

Choose a reason for hiding this comment

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

Reviewable status: 12 of 14 files reviewed, 4 unresolved discussions (waiting on @bigjools and @juledwar)


requirements/requirements.txt line 321 at r1 (raw file):

Previously, bigjools (Julian Edwards) wrote…

Grrr. dogpile-cache pulling in stuff that uses PBR.

Done.

Copy link
Collaborator

@bigjools bigjools left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @juledwar)

@0xDEC0DE 0xDEC0DE merged commit 6a5448d into master Nov 27, 2023
8 checks passed
@0xDEC0DE 0xDEC0DE deleted the issue/20 branch November 27, 2023 22:21
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 this pull request may close these issues.

3 participants