Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature/382 add nox task for dependencies update #393
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
Feature/382 add nox task for dependencies update #393
Changes from all commits
e09ced4
5b02cc6
74d9f89
ccf9963
d4ffc3f
a1734f5
864a525
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 confused by this docstring, do you keep the package versions for direct dependencies, that would make more sense for me
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.
How about naming the method
_package_versions()
or_package_version_set()
?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.
If it is a function it should contain a verb
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.
As this is a nested function, I think we can omit the underscore prefix
_
- please correct me if I'm wrong.Also, I remember @Nicoretti being quite allergic regarding the prefix
get_
as it often can be removed, and I think I could get acquainted to this opinion.I propose
pkg_version()
- short, local, without underscore_
and prefixget_
.Happy to discuss if you prefer a different name.
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.
You can use argument
encoding="utf-8"
ortext=True
to avoid using decode later on.See https://docs.python.org/3/library/subprocess.html#subprocess.run
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.
As this is a nested function, I think we can omit the underscore prefix
_
- please correct me if I'm wrong.Also, I remember @Nicoretti being quite allergic regarding the prefix
get_
as it often can be removed, and I think I could get acquainted to this opinion.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.
Please take a look at the Project keeper: Maybe it's a good idea to maintain three separate lists of updated, added, and removed dependencies and only merge or resp. render them later on when creating a report?
References in PK:
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 recommend to not execute time-consuming or potentially error raising tasks in the constructor, but rather to have a static or classmethod doing this and then only calling the constructor with prepared arguments.
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.
Please add some more information to the docstring either of the class or the proposed static method explaining the input data and/or it's expected format.
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.
Try to change to a set or more explicit start than init (not pytest-friendly)
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.
Actually, I don't understand this part.
Please let's have a peer review here.
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.
Awkward doing iteration twice
Check if can pass f directly to the extract_from_json. Or generator passing through to single to get.
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.
How about?
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 think these properties / methods can then be moved to a rendering instance.
E.g.
DependencyChanges
, but maybe even a separate one?I propose having a class
ChangeLog
orDependencyReport
.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.
How about making the changes to the attributes transparent:
This way we can even make the split method independent of the class.
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.
Please be a bit more specific or add a reference: What are "changes"? A report? A section for the changelog file? And what is meant by update dependencies? Is it in
pyproject.toml
orpoetry.lock
or both?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 also see a mismatch between the name of the nox task
dependency:audit
and the class nameDependencyUpdate
. AFAIK, auditing something is not the same as updating something.Would it be possible to stick to a single naming convention?
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.
check hint text -> should be update
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.
Please add a reference to JSONL format, e.g. https://jsonlines.org/
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.
What is basic and what is it not?
Is there also a
_perform_elaborate_vulnerability_update()
?Or is this planned to be added later on?
I propose to either rename the method (to not contain basic) or at least to add a docstring, informing about potential future additions or current limitations rendering the update to only be basic.
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.
Please ignore, if affected is incorrect in this context, and we need to stick to associated, here.
But maybe a small explanation about what associated means in contrast to affected or a link to such would be helpful, then.
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 would enjoy a literate phrasing like the following
Would that be possible / make sense?
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.
Here we see, that actually
DependencyChanges
acts as a renderer.I propose to make the properties such as
issues_not_resolved
access the internal attributes and render them for the report. Or even classDependencyChanges
could have a propertychangelog()
or a methodrender()
orprint()
?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.
Additionally, class
DependencyUpdate()
does not have a state, and is used more or less statically.So maybe we should convert it into simple functions?
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.
maybe rather in a module ->
dependencies
on level ofsecurity