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

Fix the issue #181 #182

Merged
merged 5 commits into from
Mar 13, 2019
Merged

Fix the issue #181 #182

merged 5 commits into from
Mar 13, 2019

Conversation

sttk
Copy link
Contributor

@sttk sttk commented Mar 12, 2019

This PR uses Liftoff v3.1 and moves the loading of config files before the autoloads of modules.

About adding more config flags and overriding flags from config file by ones from cli args, I will send another PRs.

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

@sttk this is awesome!!!!! Can you clean up just a couple things I commented on inline? If you want to update your commit message to match our conventional changelog style, I won't need to squash this into master.

index.js Outdated
env = mergeConfigToEnvFlags(env, cfg);
env.configProps = cfg;

this.execute(env, handleArguments);
Copy link
Member

Choose a reason for hiding this comment

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

should this be cli instead of this? - I'm thinking that will help when we switch to promises.

@@ -1,5 +1,7 @@
'use strict';

/* eslint no-console: off */
Copy link
Member

Choose a reason for hiding this comment

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

Let's just add this to the .eslintrc file

lib/versioned/^3.7.0/index.js Show resolved Hide resolved
@sttk
Copy link
Contributor Author

sttk commented Mar 13, 2019

@phated I've modified what you pointed out.

If you want to update your commit message to match our conventional changelog style, I won't need to squash this into master.

I don't want to commit my comment into master. I just wanted to use such word or style of comment. So I'm OK that you squash or modify my commit comment.
But I'll take care of commit comments to match the conventional changelog style for reducing your effort.

@phated
Copy link
Member

phated commented Mar 13, 2019

@sttk Thanks for cleaning up the commit messages and my couple of comments. I force-pushed to your branch with some nitpicks in wording. I plan to merge this into master as soon as CI finishes and I'm hoping to do some work on outstanding issues today.

@phated
Copy link
Member

phated commented Mar 13, 2019

Btw, this will also close #142 because Liftoff was modified to only log if we don't have a success

@phated phated merged commit be9d25a into gulpjs:master Mar 13, 2019
@sttk
Copy link
Contributor Author

sttk commented Mar 14, 2019

@phated Thank you so much for reviewing and merging!

@sttk sttk deleted the issue-181 branch March 14, 2019 12:27
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.

2 participants