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

feat: optional dependencies support for pip, pipenv and setuptools. #234

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

MarcusArdelean
Copy link
Contributor

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Adds support for optional dependencies/arbitrary identifiers for pip, pipenv and setuptools. The changes for poetry need to be made in snyk-poetry-lockfile-parser.

Example:
opentelemetry-distro can be imported as it is, or if we want to import some extra features that require extra dependencies we can use the following syntax: opentelemetry-distro[otlp] which will add opentelemetry-exporter-otlp as a new top level requirement.

When generating a dependency graph, both opentelemetry-distro and opentelemetry-exporter-otlp should be included as top level requirements.

What are the relevant tickets?

Support ticket SUP-2224

@MarcusArdelean MarcusArdelean self-assigned this Feb 21, 2024
@MarcusArdelean MarcusArdelean requested a review from a team as a code owner February 21, 2024 09:00
Returns:
str: `package_name` without optional_dependencies/arbitrary identifiers
"""
package_name = re.sub(r'\[[^]]*]$', '', package_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part of the code, as @JamesPatrickGill mentioned in a previous PR, is linear. I would have to rethink the way I process the data in order to not iterate over the dependencies. But I'm planning to do this when refactoring the pysrc.

pysrc/pip_resolve.py Show resolved Hide resolved
pysrc/pipfile.py Show resolved Hide resolved
pysrc/utils.py Outdated Show resolved Hide resolved
pysrc/pip_resolve.py Show resolved Hide resolved
pysrc/pip_resolve.py Outdated Show resolved Hide resolved
@dotkas
Copy link
Contributor

dotkas commented Feb 21, 2024

LGTM, nothing blocking, you can do in a follow-up PR.

@@ -246,7 +247,7 @@ def is_testable(requirement):

def detect_encoding_by_bom(path):
with open(path, 'rb') as f:
raw = f.read(4) # will read less if the file is smaller
raw = f.read(NUMBER_OF_BYTES) # will read less if the file is smaller
Copy link
Contributor

Choose a reason for hiding this comment

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

With this one I was more referring to WHY are you reading 4 bytes 😄 Maybe call it BOM_BYTES or something like that? Naming is hard.

@MarcusArdelean MarcusArdelean force-pushed the feat/optional-dependencies-support branch from 7970c8b to 03fdb5b Compare February 22, 2024 16:39
@MarcusArdelean MarcusArdelean merged commit e8a7063 into main Feb 26, 2024
22 checks passed
@MarcusArdelean MarcusArdelean deleted the feat/optional-dependencies-support branch February 26, 2024 08:07
@snyksec
Copy link

snyksec commented Feb 26, 2024

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants