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

override or extend built-in patterns #127

Merged
merged 1 commit into from
Feb 3, 2017

Conversation

bretkikehara
Copy link
Contributor

@bretkikehara bretkikehara commented Jan 29, 2017

I would like to avoid redefining the built-in gulp patterns. overridePattern enabled me to check for plugins that do not have the gulp- in its package name.

For example, webpack-stream is compatible with gulp, but doesn't match the built-in patterns. I would like to load gulp compatible plugins with gulp-load-plugins to keep all plugins consolidated.

available as @bretkikehara/gulp-load-plugins@1.5.0-alpha for testing...

@jackfranklin
Copy link
Owner

Thanks for this PR!

The code all looks great to me but I'm on the fence about accepting it primarily because it adds yet another option to the config, which is already too complicated in my opinion.

In my mind this behaviour seems to make more sense as the default, so I'm wondering if we should support this and nothing else (aka, you can't remove the default gulp patterns). That might break a lot of people's configs though, I'm not sure.

What do you think?

@bretkikehara
Copy link
Contributor Author

As is, the PR doesn't not break functionality. But i agree that overridePattern: false feels better as default. Maybe that could be a v2 change?

@jackfranklin
Copy link
Owner

jackfranklin commented Feb 3, 2017

👍 I'll release this as 1.5 and then we can consider v2. I'd love your thoughts on all the options and which ones we could remove / change. I think we've built up a bit of cruft in this module that I'd love to trim.

@jackfranklin jackfranklin merged commit fd880de into jackfranklin:master Feb 3, 2017
@jackfranklin
Copy link
Owner

Published as 1.5

@bretkikehara
Copy link
Contributor Author

bretkikehara commented Feb 4, 2017

imo avoid a major refactor b/c it will probably increase technical debt and add extra effort without much functional value. the best thing about this plugin is stability and just works, even at potentially sacrificing a bit of speed/memory.

for example, a small change could be merging rename and renameFn just by adding a typeof check.

@callumacrae
Copy link
Contributor

The entire point in the change would be to reduce technical debt - or am I misunderstanding?

@jackfranklin
Copy link
Owner

@callumacrae I'm with you, not sure I understand...

If people are happy with the plugin as is, then fine. I don't work with Gulp much these days so I don't use it - I'd much rather delegate to people like yourselves who do :)

@bretkikehara the point was if there is a breaking change that would lead to a v2, I'd rather make a lot of them in one go rather than do lots of releases with breaking changes. I think that there are too many options in the plugin and I'd love to remove them depending on what users think. Any refactor that removed some options would reduce technical debt - a refactor that increases tech debt wouldn't be a very good one?

I agree with you that rename and renameFn could be merged and I'd support that PR as a breaking change for v2.

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.

None yet

3 participants