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

[package-extractor]Add dependencies configurations field in package extractor #4255

Conversation

EscapeB
Copy link
Contributor

@EscapeB EscapeB commented Jul 28, 2023

Summary

Add "dependenciesConfigurations" field in options.
Related discussion in #4210

Details

  1. Provide "dependenciesConfigurations" field in options to configure behavior when extracting third party dependencies.
  2. Add similar fields named "patternToInclude" and "patternToExclude" to filter files inside third party dependencies.

How it was tested

Added some unit test cases to test the new field.

@EscapeB EscapeB force-pushed the feat/add_dependenciesConfigurations_field_in_package_extractor branch from 99947bb to 0b50293 Compare July 28, 2023 03:41
@EscapeB
Copy link
Contributor Author

EscapeB commented Jul 28, 2023

Hi @D4N14L, could you please help to review this PR?

libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/tests/Utils.ts Outdated Show resolved Hide resolved
@D4N14L
Copy link
Member

D4N14L commented Aug 1, 2023

I just merged my PR which should address your concerns: #4259

I'm a bit iffy on adding general 3rd party pattern include/exclude support given that it's a bit ambiguous if it applies only to the version of the package consumed directly by a project, or another dependency, or otherwise. The original issue should be addressed by my PR though.

@EscapeB
Copy link
Contributor Author

EscapeB commented Aug 2, 2023

I just merged my PR which should address your concerns: #4259

I'm a bit iffy on adding general 3rd party pattern include/exclude support given that it's a bit ambiguous if it applies only to the version of the package consumed directly by a project, or another dependency, or otherwise. The original issue should be addressed by my PR though.

I will rebase my branch~

But seems this PR can't solve my original issue, the symlink(which linked to global path) inside third party dependencies still cause errors :(

@EscapeB EscapeB force-pushed the feat/add_dependenciesConfigurations_field_in_package_extractor branch from 40ce627 to 51a3006 Compare August 2, 2023 10:56
@EscapeB EscapeB requested a review from patmill as a code owner August 2, 2023 10:56
@EscapeB
Copy link
Contributor Author

EscapeB commented Aug 14, 2023

Hi @D4N14L , what I need to do if I want to get this PR merged?

@D4N14L
Copy link
Member

D4N14L commented Aug 15, 2023

@EscapeB I'm so sorry that I've let this slide. I've been pretty busy of late and just haven't had a chance to take a closer look. Plan on doing so tomorrow when I can find a moment. Again, apologies for holding this up.

libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
common/reviews/api/package-extractor.api.md Outdated Show resolved Hide resolved
@EscapeB EscapeB force-pushed the feat/add_dependenciesConfigurations_field_in_package_extractor branch from f4340d2 to 7baa41c Compare August 18, 2023 08:01
Copy link
Member

@D4N14L D4N14L left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few more changes then I think it's good to merge.

libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
EscapeB and others added 2 commits August 21, 2023 11:18
Co-authored-by: Daniel <3473356+D4N14L@users.noreply.github.com>
@EscapeB
Copy link
Contributor Author

EscapeB commented Aug 22, 2023

Looking good! Just a few more changes then I think it's good to merge.

Hi @D4N14L, I think all the changes mentioned in the comments have now been made, please help merge this PR when you have time.

libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
libraries/package-extractor/src/PackageExtractor.ts Outdated Show resolved Hide resolved
@D4N14L D4N14L merged commit 6b9bd52 into microsoft:main Aug 22, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants