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 support for exclude rules for sources #309

Closed
wants to merge 6 commits into from
Closed

Add support for exclude rules for sources #309

wants to merge 6 commits into from

Conversation

lunny
Copy link

@lunny lunny commented Mar 20, 2020

When I use task to package a directory which includes node_modules, I found it will spend about 7 seconds even if I have set sources. And I found that's because task needs exclude rules, so I write this PR and now I can set as below:

sources:
      - "!web/.*"
      - "!web/node_modules/**"
      - "!web/tests/**"
      - "!web/dist/**"
      - "!web/mock/**"
      - "!web/LICENSE"
      - "!web/*.md"
      - "!web/**/*.md"
      - web/**
method: checksum

After the change, it will spend only 0.29s. Yeah!

@lunny lunny mentioned this pull request Mar 20, 2020
@andreynering andreynering added the type: feature A new feature or functionality. label Mar 23, 2020
Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

Hi @lunny, thanks for opening this pull request!

Unfortunately, there are changes required before this can be merged:

  • You should target v3 instead of master
  • You shouldn't touch the vendor directory (it's frozen on v3 and will be removed before releasing)
  • We definitely shouldn't use two glob libraries at the same time! I vote to try keeping the existing one if possible.

After these changes, I'll review again. 🙂

@lunny
Copy link
Author

lunny commented Mar 23, 2020

@andreynering

  1. I will change the base branch to v3
  2. I will not update vendor
  3. I found https://github.com/mattn/go-zglob/blob/a8912a37f9e79ee444e77452cba926b3e5b09a65/zglob.go#L27 , that zenv is unexported, so that it could not be saved. If we have multiple rules, it's low performance. So I suggest to drop zglob and keep the new one. But for this PR, I won't to touch other place which depends on zglob. So that it's easier to review. On further PRs, zglob could be replaced totally.

@lunny lunny changed the base branch from master to v3 March 23, 2020 02:53
@lunny
Copy link
Author

lunny commented Apr 19, 2020

@andreynering done.

@andreynering
Copy link
Member

Closed by mistake, sorry

@andreynering andreynering reopened this Aug 17, 2020
@andreynering andreynering changed the base branch from v3 to master August 17, 2020 00:02
@marco-m
Copy link
Contributor

marco-m commented Aug 29, 2020

Hello, what is the status of this PR? From #225 I understand it is complicated, but I have hope :-)

@butuzov
Copy link
Contributor

butuzov commented Mar 12, 2021

@andreynering is there any updates on this issue? Having something to exclude is quite a nice feature to have.

@tamasfe
Copy link

tamasfe commented Mar 26, 2021

@andreynering is there anything I could do to help with driving this forward? It would be a crucial feature for me as currently code generation patterns (e.g. generating **/*.gen.go based on **/*.go) do not seem possible without workarounds.

@kevpie
Copy link

kevpie commented Apr 8, 2021

@andreynering is there anything I could do to help with driving this forward? It would be a crucial feature for me as currently code generation patterns (e.g. generating **/*.gen.go based on **/*.go) do not seem possible without workarounds.

How promising does helping getting mattn/go-zglob#30 landed go towards solving this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature or functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants