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.

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

feat: add flags to ignore files/directories #222

Closed
wants to merge 2 commits into from
Closed

feat: add flags to ignore files/directories #222

wants to merge 2 commits into from

Conversation

1bitphoenix
Copy link
Contributor

Add '--ignore-dir' flag to not include directories and '--ignore' flag
to not include files while asar packaging

Signed-off-by: Rituka Patwal [email protected]

@erickzhao erickzhao requested a review from a team September 20, 2021 15:58
@1bitphoenix 1bitphoenix force-pushed the create-ignore-dir-files-flags branch 2 times, most recently from cf62119 to 5eef42e Compare September 21, 2021 15:08
@1bitphoenix
Copy link
Contributor Author

@erickzhao @malept
If anyone could review this PR? Not sure who should I tag here.

@1bitphoenix
Copy link
Contributor Author

@zcbenz @kevinsawicki
Can anyone review this PR? Not sure who should I tag here.

ckerr
ckerr previously requested changes Oct 19, 2021
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

The idea and code both look fine, thank you for this patch!

However it doesn't look like you added anything to test/ to exercise these code paths. Would it be possible to add some tests for this feature?

@malept
Copy link
Member

malept commented Oct 19, 2021

Generally speaking, instead of adding a new pair of options, I'd prefer to have unpack and unpackDir be enhanced to take an array of globs in addition to a single glob. The glob module should be able to specify ignores (just like the files array in package.json).

@1bitphoenix 1bitphoenix force-pushed the create-ignore-dir-files-flags branch 3 times, most recently from b8cff26 to 7c08de2 Compare February 7, 2022 08:22
@1bitphoenix
feat: add flags to ignore files/directories
48ec37d 
Add '--ignore-dir' flag to not include directories and '--ignore' flag
to not include files while asar packaging

Signed-off-by: Rituka Patwal <[email protected]>
@1bitphoenix
Copy link
Contributor Author

1bitphoenix commented Feb 7, 2022 •
edited
Loading

The idea and code both look fine, thank you for this patch!

However it doesn't look like you added anything to test/ to exercise these code paths. Would it be possible to add some tests for this feature?

Hi @ckerr,
I have updated the PR by adding some tests. PTAL.

(PS: Sorry for the delay.)

@1bitphoenix
Copy link
Contributor Author

Generally speaking, instead of adding a new pair of options, I'd prefer to have unpack and unpackDir be enhanced to take an array of globs in addition to a single glob. The glob module should be able to specify ignores (just like the files array in package.json).

@malept
I'm a little lost on how can we figure out from the list of globs which glob user wishes to unpack and which glob user wishes to ignore. Can you please explain your thoughts in detail?

@1bitphoenix
Copy link
Contributor Author

@ckerr
Just a friendly ping. Can you review the PR with tests added?

@erickzhao erickzhao marked this pull request as draft November 7, 2023 22:43
@erickzhao erickzhao dismissed ckerr’s stale review November 7, 2023 22:44

stale review, tests were added

@dsanders11 dsanders11 removed their assignment Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr left review comments

Assignees

@erickzhao erickzhao

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@1bitphoenix @malept @ckerr @dsanders11 @MarshallOfSound @erickzhao