added logging on install #2234
Merged
added logging on install #2234
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Plamen5kov
force-pushed
the
plamen5kov/fix_python_test
branch
3 times, most recently
from
3b3cf55 to
6e7a00e
Compare
|
run CI |
1 similar comment
|
run CI |
rosen-vladimirov
suggested changes
Nov 18, 2016
| try { | ||
| viewResult = this.$childProcess.exec(`npm view ${packageName} ${flags}`).wait(); | ||
| } catch(e) { | ||
| this.$logger.out(e); |
There was a problem hiding this comment.
What's the reason to catch the error, print it and throw it again? In case npm view is used in a command, CLI's commandsService will print the error when it's thrown.
There was a problem hiding this comment.
this is temporary, it will failWithoutHelp
| @@ -70,6 +70,14 @@ export class NpmInstallationManager implements INpmInstallationManager { | |||
|
|
|||
| private installCore(packageName: string, pathToSave: string, version: string, dependencyType: string): IFuture<string> { | |||
| return (() => { | |||
| if(packageName.indexOf(".") === 0 || packageName.indexOf(".tgz") >= 0) { | |||
There was a problem hiding this comment.
this will fail in case user passes path without ./ in front, for example in case I want to install local directory I can pass ./my-dir or my-dir. Both will work
| @@ -70,6 +70,14 @@ export class NpmInstallationManager implements INpmInstallationManager { | |||
|
|
|||
| private installCore(packageName: string, pathToSave: string, version: string, dependencyType: string): IFuture<string> { | |||
| return (() => { | |||
| if(packageName.indexOf(".") === 0 || packageName.indexOf(".tgz") >= 0) { | |||
| let cwd = process.cwd(); | |||
| let possiblePackageName = path.join(`${cwd}`, packageName); | |||
There was a problem hiding this comment.
instead of this you can use path.resolve(packageName) - this will work with both full and relative paths. In this case you can skip the above if as well and use:
const possiblePackageName= path.resolve(packageName);
if (this.$fs.exists(possiblePackageName).wait()) {
...
}
rosen-vladimirov
approved these changes
Nov 18, 2016
|
run CI |
|
Related issue #282 |
|
run CI |
1 similar comment
|
run CI |
Plamen5kov
force-pushed
the
plamen5kov/fix_python_test
branch
from
49dba01 to
9148c5d
Compare
…/remove_npm2_dependency"" This reverts commit a60eabd.
Plamen5kov
force-pushed
the
plamen5kov/fix_python_test
branch
from
9148c5d to
fc430b8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixing python tests