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

T/296: Remove gulp dependency. #298

Merged
merged 12 commits into from
Nov 13, 2017
Merged

T/296: Remove gulp dependency. #298

merged 12 commits into from
Nov 13, 2017

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Oct 13, 2017

Suggested merge commit message (convention)

Other: Remove gulp dependency. Closes #296.

Now all packages are independent of gulp. Depending on usage you might either create "script " entry in pacakge.json to invoke bin executables or require library into script.

  • Package ckeditor5-dev-env exposes new translations binary.
  • Removed gulp-jsdoc3 from building API documentation. Now jsdoc is invoked directly.
  • Removed options param from logger methods. Logger no longer uses gutil.log method.

BREAKING CHANGE: Gulp is no longer required to run tasks from ckeditor5-dev-* packages use npm scripts instead.
BREAKING CHANGE: Gulp task are removed. New npm scripts were introduced.


Required for ckeditor/ckeditor5#602.

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

LGTM. I left one question and one note.

try {
tools.shExec( `${ cmd } -c ${ tmpConfig.name }`, { verbosity: 'info' } );
} catch ( error ) {
return reject( 'Error during JSDoc generation.' );
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to attach message from error. It should help understand what went wrong.

@@ -34,6 +34,9 @@
"node": ">=6.0.0",
"npm": ">=3.0.0"
},
"bin": {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you extract translations tasks from env/index.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could either leave it as is or create a bin script (translations). I didn't see a point of duplicating require().*Translation() in ckeditor5 repo.

This script accepts collect, upload or download param.

@Reinmar
Copy link
Member

Reinmar commented Nov 11, 2017

BTW, @jodator, please configure your editor to trim trailing whitespaces.

@jodator
Copy link
Contributor Author

jodator commented Nov 12, 2017

@Reinmar double checked that now - currently I have this on. Might be off for some time as I've reset my editor settings during that time.

@Reinmar Reinmar merged commit 723bee5 into master Nov 13, 2017
@Reinmar Reinmar deleted the t/296 branch November 13, 2017 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants