-
Notifications
You must be signed in to change notification settings - Fork 185
Make pipenv strategy differentiate between direct and transitive deps #1601
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
Conversation
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 looks good! All the comments I wrote are optional.
Could you update the Changelog.md
file though? I don't see any reason not to release this. I can help you through that process if you haven't done it yet.
PipfileToml | ||
(pipfilePackageList ["pkgOne"]) | ||
(pipfilePackageList ["pkgTwo"]) |
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.
[Optional] We use records positionally a lot in the CLI. It quickly becomes unwieldy and hard to read when it's a record with many fields. If I wrote a function with seven arguments or something you'd probably, correctly, call it out in a review. Personally, I'd like it if we all moved towards being a bit more explicit.
There's even an action HLS has to do it for you.
This particular case looks fine to me if you want to keep it though, or disagree with the idea above.
addWithEnv env sourcesMap name dep = do | ||
let pkg = PipPkg name (Text.drop 2 <$> fileDepVersion dep) | ||
-- Use the Pipfile as the source of truth for which dependencies are direct | ||
let graphFn = if Map.member name pipfilePackages || Map.member name pipfileDevPackages then direct else deep |
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] It may be more straightforward to just inline this if expression.
Overview
This PR fixes the pipenv strategy to differentiate between direct and transitive dependencies. We are able to make this distinction via static analysis, but generating edges requires the
pipenv graph
command, and is thus only available as a dynamic strategy. NeitherPipfile.lock
norpipenv graph
adequately identify which dependences are direct, so I've introduced parsing ofPipfile
in order to accurately make this distinction.I also included a fix for some broken links in the Swift docs.
Acceptance criteria
Pipenv correctly differentiates between direct and transitive dependencies.
Testing plan
requests
package. Confirm that the dependency graph looks correctlangchain
as well which itself also depends onrequests
. Confirm thatlangchain
andrequests
are both reported as direct dependencies.Risks
Metrics
References
Checklist
docs/
.docs/README.ms
and gave consideration to how discoverable or not my documentation is.Changelog.md
. If this PR did not mark a release, I added my changes into an## Unreleased
section at the top..fossa.yml
orfossa-deps.{json.yml}
, I updateddocs/references/files/*.schema.json
AND I have updated example files used byfossa init
command. You may also need to update these if you have added/removed new dependency type (e.g.pip
) or analysis target type (e.g.poetry
).docs/references/subcommands/<subcommand>.md
.