CHE-2214 NodeJs Debugger #2625
CHE-2214 NodeJs Debugger #2625
Conversation
|
@benoitf |
| @@ -0,0 +1,58 @@ | |||
| # Eclipse # | |||
There was a problem hiding this comment.
Why do you need your own .gitignore here?
| <parent> | ||
| <artifactId>che-plugin-nodejs-debugger-parent</artifactId> | ||
| <groupId>org.eclipse.che.plugin</groupId> | ||
| <version>5.0.0-M2-SNAPSHOT</version> |
There was a problem hiding this comment.
Please update version of yours modules to 5.0.0-M5-SNAPSHOT
| </dependency> | ||
| </dependencies> | ||
| <build> | ||
| <testSourceDirectory>src/test/java</testSourceDirectory> |
There was a problem hiding this comment.
I think you can remove testSourceDirectory, outputDirectory, resources sections because there all are default
|
|
||
| findFileInProject(project.get(), location, callback); | ||
|
|
||
| // final EditorPartPresenter activeEditor = editorAgent.getActiveEditor(); |
There was a problem hiding this comment.
Do you need these commented lines?
| * | ||
| * @author Anatolii Bazko | ||
| */ | ||
| public interface NodeJsDebuggerLocalizationConstant extends com.google.gwt.i18n.client.Messages { |
There was a problem hiding this comment.
Why you can't use import for Messages class?
| </encoder> | ||
| </appender> | ||
|
|
||
| <root level="DEBUG"> |
There was a problem hiding this comment.
Are you sure that you need DEBUG logging level here? I prefer to use ERROR =
| </encoder> | ||
| </appender> | ||
|
|
||
| <root level="INFO"> |
There was a problem hiding this comment.
ERROR logging level would be better
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/545/ |
| <artifactId>license-maven-plugin</artifactId> | ||
| <configuration> | ||
| <excludes> | ||
| <exclude>**/*.png</exclude> |
There was a problem hiding this comment.
do we need a custom exclude pattern ? (and not move this setting to the global configuration ?)
| LOG.error("NodeJs failed to stop"); | ||
| } | ||
| } catch (InterruptedException ignored) { | ||
| } |
There was a problem hiding this comment.
we should at least log it no ?
| LOG.error(String.format("Command execution <%s> failed", command), e); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
why do we have sleep instruction there ?
There was a problem hiding this comment.
Agree. Can be removed.
| } | ||
|
|
||
| try { | ||
| sleep(100); |
There was a problem hiding this comment.
Outdated code. Should be reworked.
|
Build # 551 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/551/ to view the results. |
| sb.append(connectionInfo); | ||
| } | ||
| } catch (IllegalArgumentException ignored) { | ||
| } |
There was a problem hiding this comment.
maybe some log here or comment why you skip exception?
There was a problem hiding this comment.
No value, I will add comment in the code
| @Override | ||
| public void run() { | ||
| try { | ||
| InputStream in = getInput(); |
There was a problem hiding this comment.
should you close stream?
There was a problem hiding this comment.
No, when debug process is stopped, then the stream will be stopped also.
|
|
||
| String newTarget; | ||
| String[] target = location.getTarget().split(":"); | ||
| if (target[0].equals("scriptId")) { |
There was a problem hiding this comment.
ok, I will handle it.
| final List<Breakpoint> breakpoints = new ArrayList<>(); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> m = GSON.fromJson(nodeJsOutput.getOutput(), Map.class); |
There was a problem hiding this comment.
Can you use JsonObject and get value by key?
|
Build # 558 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/558/ to view the results. |
|
Build # 562 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/562/ to view the results. |
What does this PR do?
Provides ability to debug nodejs applications.
What issues does this PR fix or reference?
#2214
Known limitations:
Please review Che's Contributing Guide for best practices.