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: support io.Reader based parsers #451

Merged
merged 11 commits into from
Aug 18, 2023
Merged

feat: support io.Reader based parsers #451

merged 11 commits into from
Aug 18, 2023

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Jul 26, 2023

Resolves #176

@G-Rath

This comment was marked as resolved.

@another-rex
Copy link
Collaborator

I think we need to discuss how you want to handle transitioning from "cli" to "library" given that everything there is currently "parse-as"; I think we could either keep it as is, go for a hybrid approach, or rename everything non-breaking to "extract-as".

I think the CLI actually doesn't mention "parse" at all right? To specify a parsing method we are using the Cargo.lock:/path/to/Cargo.lock, and taking a look at osvscanner.go, the parse and parseAs mentions are argument names are all private variables / argument names.

I think we can go ahead and rename non breaking things to extract, but haven't looked into it enough to see how practical that is.

The part where the code actually suffers currently is in FindExtractor, because the Gradle extractor has two different files but the Extractor interface doesn't support convoying that back - for now I've just duplicated the ShouldExtract into the FindExtractor code.

Hmm I'm not clear on what you mean here. I had a look at FindExtractor in this PR, and I don't think stabilizeFindExtractorName is necessary, since both gradle files are parsed the same, we can just decide on a parser/extractor name and return that, unless I am missing something?

@G-Rath

This comment was marked as resolved.

@G-Rath G-Rath marked this pull request as ready for review August 15, 2023 21:26
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.

Looks pretty good! Just some minor comments.

pkg/lockfile/parse.go Outdated Show resolved Hide resolved
pkg/lockfile/parse-gradle-lock.go Show resolved Hide resolved
Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

nice, thanks @G-Rath !!! looks pretty good overall.

pkg/lockfile/parse-gradle-lock.go Show resolved Hide resolved

lockfileContents, err := os.ReadFile(pathToLockfile)
func (e PipenvLockExtractor) ShouldExtract(path string) bool {
return filepath.Base(path) == "Pipfile.lock"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth creating constants for these names for all extractors, and using them in both ShouldExtract and init ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Practically no because I don't see a strong value in doing that now since it's not like it'll make the code faster or anything, and architecturally I'd argue you're conflating "extractor names" with "file paths"

pkg/lockfile/extractor.go Show resolved Hide resolved
@another-rex another-rex merged commit b894876 into google:main Aug 18, 2023
7 checks passed
@G-Rath G-Rath deleted the support-io-reader branch January 22, 2024 19:15
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.

Add io.Reader variants to lockfile package
3 participants