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

feat!: Add extends syntax #103

Merged
merged 23 commits into from
Oct 24, 2021
Merged

feat!: Add extends syntax #103

merged 23 commits into from
Oct 24, 2021

Conversation

phated
Copy link
Member

@phated phated commented Apr 21, 2019

@sttk this is my (very hacky) initial work on my vision for extends syntax. Please review and make any suggestions. I still need to find answers on all the TODOs and write a ton of tests (which will likely result in refactoring).

Closes #101 (supersedes it)

@sttk
Copy link
Contributor

sttk commented Apr 22, 2019

@phated This pr is generally good! I'm sorry that I'm slow on the uptake. Wrong idea (and my poor english) had hindered my understanding. As you commented, findUp option of fined can solve the issue of loading a config file in cwd found up.

BTW, there are one thing I'd like you to confirm. Will the way to specify .configFiles of constructor be changed to use array? (I think it's better for those order.)

@phated
Copy link
Member Author

phated commented Apr 22, 2019

@sttk don't worry about it! Your work heavily influenced this code.

BTW, there are one thing I'd like you to confirm. Will the way to specify .configFiles of constructor be changed to use array? (I think it's better for those order.)

I think it's important to switch the configFiles values to be array because we are now loading and merging the configs. My current implementation will look at each entry until one is found, then it will load that file and switch to merging using extends property.

test/index.js Outdated Show resolved Hide resolved
@coveralls

This comment has been minimized.

@phated phated changed the title WIP: extends syntax feat!: Add extends syntax Oct 19, 2021
@phated
Copy link
Member Author

phated commented Oct 19, 2021

@sttk this has taken me a long time to come back to, but I am hoping you can review this PR because it adds the extends syntax that we plan to support in gulp-cli. Thanks 🙏

@phated phated requested a review from sttk October 19, 2021 00:03
@phated
Copy link
Member Author

phated commented Oct 19, 2021

Oh, and this is a breaking change because we changed the syntax to an array of fined objects (instead of a single object).

@sttk
Copy link
Contributor

sttk commented Oct 21, 2021

@phated OK, I'll review this PR this weekend.

Copy link
Contributor

@sttk sttk left a comment

Choose a reason for hiding this comment

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

@phated I've reviewd this PR, and I didn't find any problem. Great!

@phated phated merged commit 1ae81d0 into master Oct 24, 2021
@phated phated deleted the config-extends branch October 24, 2021 14:28
@github-actions github-actions bot mentioned this pull request Oct 24, 2021
phated added a commit that referenced this pull request Nov 21, 2021
phated added a commit that referenced this pull request Nov 21, 2021
phated added a commit that referenced this pull request Nov 22, 2021
phated added a commit that referenced this pull request Nov 22, 2021
phated added a commit that referenced this pull request Nov 22, 2021
phated added a commit that referenced this pull request Nov 22, 2021
phated added a commit that referenced this pull request Nov 22, 2021
This was referenced Nov 22, 2021
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.

3 participants