Skip to content

Adds support for pubspec.lock files #104

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 26 commits into from
Closed

Adds support for pubspec.lock files #104

wants to merge 26 commits into from

Conversation

jhutchings1
Copy link
Contributor

This PR adds support for pubspec.lock files, which are used in Flutter and Dart projects.

@jhutchings1 jhutchings1 requested a review from a team as a code owner April 17, 2022 06:01
@jhutchings1 jhutchings1 requested a review from 8Gitbrix April 17, 2022 06:01
@JamieMagee JamieMagee added version:minor type:feature Feature (new functionality) labels Apr 18, 2022
@JamieMagee JamieMagee self-requested a review May 2, 2022 21:53
cobya
cobya requested changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@cobya cobya left a comment

Choose a reason for hiding this comment

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

Apologies for the late review here, our team has been managing other priorities.

src/Microsoft.ComponentDetection.Detectors/pub/PubComponentDetector.cs
.WithNamingConvention(UnderscoredNamingConvention.Instance)
.Build();

dynamic pubspec = deserializer.Deserialize<System.Dynamic.ExpandoObject>(text);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the deserialization contract be captured in a class like https://github.com/microsoft/component-detection/blob/main/src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmYaml.cs so we aren't using dynamic objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that in an earlier commit, and my testing was running into parsing failures because the lock files weren't consistent enough. The dynamic object seems to be more reliable, but I may have missed something. jhutchings1@829c548

@jhutchings1
Copy link
Contributor Author

@cobya I think I've addressed most of your outstanding feedback on this one. Do you want to have another look?

cobya
cobya reviewed Aug 30, 2022
View reviewed changes
@JamieMagee
Copy link
Member

We should also add some test cases to verification tests

@FernandoRojo
Copy link
Contributor

Going through some older PRs now, do we still want this to get merged in @jhutchings1

@jhutchings1
Copy link
Contributor Author

Whoa, this is an old one. I would look to @jonjanego and @ncouraud on this one. I'm not sure if there have been breaking changes to the formats in the last few years.

@FernandoRojo
Copy link
Contributor

FernandoRojo commented Oct 28, 2024 •
edited
Loading

Closing this PR just because it's duplicated in #888,

But I can merge the code from this PR into there if we can confirm that there haven't been breaking changes/ it is still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamieMagee JamieMagee JamieMagee approved these changes

@cobya cobya cobya approved these changes

@8Gitbrix 8Gitbrix Awaiting requested review from 8Gitbrix 8Gitbrix is a code owner automatically assigned from microsoft/ose-component-detection-maintainers

Assignees
No one assigned
Labels
type:feature Feature (new functionality) version:minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jhutchings1 @JamieMagee @FernandoRojo @cobya @amitla1