-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(python): skip dev group's deps for poetry #8106
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
[dependency-groups]
test = ["pytest<8", "coverage"]
typing = ["mypy==1.7.1", "types-requests"]
lint = ["black", "flake8"]
dev = [{include-group = "test"}, {include-group = "typing"}, {include-group = "lint"}] |
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
81d513f
to
7de61c8
Compare
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. Left 2 small comments
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
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
cc. @knqyf263
func (d *dependencies) UnmarshalTOML(data any) error { | ||
m, ok := data.(map[string]any) | ||
if !ok { | ||
return fmt.Errorf("dependencies must be map, but got: %T", data) |
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.
nit: for consistency and stacktrace
return fmt.Errorf("dependencies must be map, but got: %T", data) | |
return xerrors.Errorf("dependencies must be map, but got: %T", data) |
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.
Fixed d1e2450
return nil | ||
} | ||
|
||
func (a poetryAnalyzer) parsePyProject(fsys fs.FS, path string) (map[string]any, error) { | ||
func filterProdPackages(project pyproject.PyProject, app *types.Application) { |
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.
We should flag the Dev field instead of filtering for --include-dev-deps
, but if you want to keep the original behavior in this PR, we can add the change in a separate PR.
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.
@knqyf263 as i wrote in #8080 (comment) - are you sure that we want to add dev deps under Dev
field?
Previously we only did this upon request from users.
I have yet to meet a python user who needs to scan for dev dependencies
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.
Hmm. It sounds reasonable, but we should add a new column showing if --include-dev-deps is supported.
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.
But if the implementation cost is not so different, it's better to mark dev dependencies.
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'll open another PR for this one
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Description
This PR skips dependencies from the
dev
group for the poetry.Related issues
Checklist