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

Poetry conversion #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Poetry conversion #10

wants to merge 4 commits into from

Conversation

colton-herrod-bayer
Copy link
Collaborator

Converting to use poetry, more modern code formatting tools, updating various dependencies, deprecated libraries, and other cleanup.

@colton-herrod-bayer
Copy link
Collaborator Author

Its an open question on if we even want to do this. Poetry is one of our internal standards and using it in our owned OSS projects makes things easier for us. It is a bit of a shift for the project itself, though.

Input from the rest of the team would be useful here.

import yaml

#
file_manager = ExitStack()
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is not clear to me what we are doing with this. aren't cotnext managers usually used in 'with' blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is part of the recommended migration from pkg_resources to importlib. This is the suggested pattern for files that need to persist for longer than a with block should be set up for. See here.

That said, I think I missed the bits needed to close the file. I'll get that set up shortly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see, it looks like you might only need the ExitStack when you need the file to stick around for the life of the program and outside the context of the with. however, we just parse them and store the python objects in memory. so i think we can simply use the 'with' keyword. you might be able to avoid the file(s) and use https://importlib-resources.readthedocs.io/en/latest/migration.html#pkg-resources-resource-stream to pass a stream directly to json.load()/yaml.safe_load() too

from . import expand_action
from .finding import Finding
from .misc import make_list
from .statement import Statement


class Policy:
_findings = []
policy_json = None
_findings: list[Finding] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

im not sure these class variables serve any purpose... we can probably remove them and move the typing to the init's instance variables

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know that they do either, but I'm inclined to leave that sort of change to a separate chunk of work. Do we want to track that in the repository?

Copy link
Collaborator

@matthew-climate matthew-climate left a comment

Choose a reason for hiding this comment

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

lgtm. those resources are being cleaned up but you can narrow the scope on them probably

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