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

Add pdm lockfile support #776

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Add pdm lockfile support #776

merged 1 commit into from
Jan 31, 2024

Conversation

jtt
Copy link
Contributor

@jtt jtt commented Jan 29, 2024

Add support for parsing package information from pdm.lock -files used by pdm, package and dependency manager for Python (https://pdm-project.org/latest/)

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looks good, just got a few changes - would you mind also adding an "empty.lock" (which is "no packages", not "completely empty file") and "two-packages.lock" fixtures as well? (I know technically your other tests cover this, but it's consistent with the standard set of fixtures we have for our other lockfiles)

Copy link
Collaborator

Choose a reason for hiding this comment

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

would you mind restructuring these to use individual functions to be consistent with the rest of our lockfile tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restructured

Comment on lines 23 to 25
const PdmLockFileName = "pdm.lock"
const pdmGroupDefault = "default"
const pdmGroupDev = "dev"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't typically use constants for these, and you're only referencing them each once so lets just inline them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed constants

type PdmLockPackage struct {
Name string `toml:"name"`
Version string `toml:"version"`
Groups []string `toml:"groups"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

just noting for others: it looks like pdm only very recently started recording dependency groups at the package level in its lockfiles - I couldn't actually find any locks searching GitHub, but the PRs on pdm support this feature as being added

Comment on lines 71 to 72
}

//nolint:gochecknoinits
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should ensure the extractor matches the interface:

Suggested change
}
//nolint:gochecknoinits
}
var _ Extractor = PdmLockExtractor{}
//nolint:gochecknoinits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

const pdmGroupDefault = "default"
const pdmGroupDev = "dev"

type PdmExtractor struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This matches the naming we use for the other extractors:

Suggested change
type PdmExtractor struct{}
type PdmLockExtractor struct{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the name

@jtt
Copy link
Contributor Author

jtt commented Jan 29, 2024

Looks good, just got a few changes - would you mind also adding an "empty.lock" (which is "no packages", not "completely empty file") and "two-packages.lock" fixtures as well? (I know technically your other tests cover this, but it's consistent with the standard set of fixtures we have for our other lockfiles)

Added requested tests. Amended fixes to the existing commit.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Awesome stuff! It'd be great if you could space out the tests a bit, but I think this is good to go!

}

func TestParsePdmLock_FileDoesNotExist(t *testing.T) {
t.Parallel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these are pretty squashed together

usually I like to have a new line between every function, after t.Parallel(), and before the first expect* call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added newlines to test functions.

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cbdceae) 79.85% compared to head (3b6e492) 80.02%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #776      +/-   ##
==========================================
+ Coverage   79.85%   80.02%   +0.17%     
==========================================
  Files          90       91       +1     
  Lines        6174     6213      +39     
==========================================
+ Hits         4930     4972      +42     
+ Misses       1040     1038       -2     
+ Partials      204      203       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Adds support for parsing package version information from lockfiles of pdm,
package and dependency manager for Python.
Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Thanks!

@another-rex another-rex merged commit c050cea into google:main Jan 31, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants