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.

Sign up for GitHub

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

Jump to bottom

Use tsconfig.json data in gulpfile.js instead of hardcoded values. #572

Merged
merged 1 commit into from
Mar 11, 2017
Merged

Use tsconfig.json data in gulpfile.js instead of hardcoded values. #572

merged 1 commit into from
Mar 11, 2017

Conversation

AndrienkoAleksandr
Copy link
Contributor

Use tsconfig.json data in gulpfile.js instead of hardcoded values. Little code clean up

Signed-off-by: Aleksandr Andrienko aandrienko@codenvy.com

@coveralls
Copy link

Coverage Status

Coverage remained the same at 64.637% when pulling 4afb3fc on AndrienkoAleksandr:gulpImprove into 3e5fcf7 on sourcelair:master.

Tyriar
Tyriar reviewed Feb 28, 2017
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor comment.

gulpfile.js
@@ -14,27 +14,28 @@ const ts = require('gulp-typescript');


let buildDir = process.env.BUILD_DIR || 'build';

let tsProject = ts.createProject('tsconfig.json');
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you're not just requiring the json file here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is because tsProject.src() is being used below by Gulp.

This was the case also before, but @AndrienkoAleksandr seems to have brought createProject at the top level, to be able to access it's settings inside the tasks.

parisk
parisk suggested changes Mar 2, 2017
View reviewed changes
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

@AndrienkoAleksandr can you please rebase with master, before we are able to merge this?

gulpfile.js
@@ -14,27 +14,28 @@ const ts = require('gulp-typescript');


let buildDir = process.env.BUILD_DIR || 'build';

let tsProject = ts.createProject('tsconfig.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is because tsProject.src() is being used below by Gulp.

This was the case also before, but @AndrienkoAleksandr seems to have brought createProject at the top level, to be able to access it's settings inside the tasks.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 67.468% when pulling c618f16 on AndrienkoAleksandr:gulpImprove into ec9608d on sourcelair:master.

@parisk
Copy link
Contributor

parisk commented Mar 6, 2017

@AndrienkoAleksandr can you please rebase with master, in order to merge this?

@AndrienkoAleksandr
Use tsconfig.json data in gulpfile.js instead of hardcoded values.
dc3a136 
Use tsconfig.json data in gulpfile.js instead of hardcoded values. Little code clean up

Signed-off-by: Aleksandr Andrienko <aandrienko@codenvy.com>
@coveralls
Copy link

Coverage Status

Coverage decreased (-7.3%) to 60.475% when pulling dc3a136 on AndrienkoAleksandr:gulpImprove into c9ad3a5 on sourcelair:master.

@AndrienkoAleksandr
Copy link
Contributor Author

@parisk branch rebased.

@Tyriar
Copy link
Member

Tyriar commented Mar 6, 2017

Let's wait until 2.4 is released before we merge this (just in case).

@Tyriar Tyriar added this to the 2.5.0 milestone Mar 6, 2017
@Tyriar Tyriar closed this Mar 8, 2017
@Tyriar Tyriar reopened this Mar 8, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-7.3%) to 60.46% when pulling dc3a136 on AndrienkoAleksandr:gulpImprove into c9ad3a5 on sourcelair:master.

@parisk
Copy link
Contributor

parisk commented Mar 11, 2017

Looks great, thanks!

@parisk parisk merged commit ff29fd6 into xtermjs:master Mar 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tyriar Tyriar Tyriar left review comments

@parisk parisk parisk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AndrienkoAleksandr @coveralls @parisk @Tyriar