-
Notifications
You must be signed in to change notification settings - Fork 255
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: expose build-system dependencies #319
Conversation
1386faf
to
99e8da4
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Kudos, SonarCloud Quality Gate passed! |
2ae4888
to
7bd850e
Compare
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request exposes the build-system dependencies of a Poetry project as Sequence diagram for build system dependency resolutionsequenceDiagram
participant Poetry
participant PyProjectTOML
participant Dependency
Poetry->>PyProjectTOML: Get build_system
PyProjectTOML-->>Poetry: Return build system config
loop For each requirement
Poetry->>Dependency: create_from_pep_508(requirement)
alt PEP 508 parsing successful
Dependency-->>Poetry: Return Dependency instance
else PEP 508 parsing failed
Note over Poetry: Try as file/directory path
alt Is file
Poetry->>Poetry: Create FileDependency
else Is directory
Poetry->>Poetry: Create DirectoryDependency
end
end
end
Class diagram showing Poetry class modificationsclassDiagram
class Poetry {
-_build_system_dependencies: list[Dependency]
+pyproject: PyProjectTOML
+package: ProjectPackage
+local_config: dict[str, Any]
+build_system_dependencies: list[Dependency]
+get_project_config(config: str, default: Any): Any
}
class Dependency {
+create_from_pep_508(requirement: str): Dependency
}
class FileDependency {
+name: str
+path: Path
}
class DirectoryDependency {
+name: str
+path: Path
}
Poetry --> "*" Dependency : has
FileDependency --|> Dependency : extends
DirectoryDependency --|> Dependency : extends
note for Poetry "New property build_system_dependencies
parses requirements into Dependency objects"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @abn - I've reviewed your changes - here's some feedback:
Overall Comments:
- The test appears to have issues: there's a typo in the parameter name 'requries' and the test implementation doesn't match the parametrized cases - it only tests 'sample_project' while declaring two different test scenarios.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Just a small thing.
7bd850e
to
13ad55a
Compare
@sourcery-ai review |
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.
Hey @abn - I've reviewed your changes - here's some feedback:
Overall Comments:
- There appears to be a bug in the logging - the 'Skipping build system dependency' debug message is logged when a dependency is successfully added, which is incorrect. The log message should either be moved to the else branch when skipping, or the message should be changed to indicate successful addition.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
13ad55a
to
f0c2db9
Compare
This change allows for a Poetry project instance's build-system requirements to be accessed as
Dependency
instances. Preserving useful parsing from #58 as original PR was dropped.Summary by Sourcery
New Features:
Dependency
instances.