chore: rename nodereport to node-report #37

gdams · 2017-01-12T16:10:08Z

This changes nodereport to node-report

rnchamberlain · 2017-01-12T16:53:03Z

@gdams the hyphen is not allowed in C variable or macro names, so the project won't build as-is. For example, NODEREPORT_VERSION will need to change to perhaps NODE_REPORT_VERSION, not node-report_VERSION. Ditto C variables and the environment variables.

richardlau

I assume this was a global search and replace. It's left very ugly looking function/variable names, e.g. Processnode-reportFileName has replaced ProcessNodeReportFileName.

richardlau · 2017-01-12T17:03:35Z

binding.gyp

@@ -21,7 +21,7 @@
        }],
      ],
      "defines": [
-        'NODEREPORT_VERSION="<!(node -p \"require(\'./package.json\').version\")"'
+        'node-report_VERSION="<!(node -p \"require(\'./package.json\').version\")"'


Please either revert this, or upper case. I don't believe - is a valid character for C/C++ macro names.

richardlau · 2017-01-12T17:04:32Z

CHANGES.md

@@ -1,7 +1,7 @@
 2016-12-12, Version 1.0.7
 =========================

- * Fix version reporting in NodeReport section (Richard Lau)
+ * Fix version reporting in node-report section (Richard Lau)


I don't think we should rename in the history -- It was correct at the time.

richardlau · 2017-01-12T17:04:40Z

CHANGES.md

@@ -23,7 +23,7 @@

 * Fix for failure in fatal error (OOM) test (Richard Chamberlain)

- * Add support for nodereport on AIX (Richard Chamberlain)
+ * Add support for node-report on AIX (Richard Chamberlain)


I don't think we should rename in the history -- It was correct at the time.

richardlau · 2017-01-12T17:06:56Z

LICENCE.md

 --------------------------------------------------

-*nodereport contributors listed at <https://github.com/nodejs/nodereport/blob/master/AUTHORS>*
+*node-report contributors listed at <https://github.com/nodejs/node-report/blob/master/AUTHORS>*


If we're updating this file, https://github.com/nodejs/nodereport/blob/master/AUTHORS no longer exists (it was removed by #13).

richardlau · 2017-01-12T17:14:03Z

README.md

+export node-report_FILENAME=stdout|stderr|<filename>
+export node-report_DIRECTORY=<full path>
+export node-report_COREDUMP=yes|no
+export node-report_VERBOSE=yes|no


We want to keep the environment variables case (upper), probably without the hyphen.

richardlau · 2017-01-12T17:15:09Z

index.js

-// NODEREPORT_EVENTS env var overrides the defaults
-const options = process.env.NODEREPORT_EVENTS || 'exception+fatalerror+signal+apicall';
+// node-report_EVENTS env var overrides the defaults
+const options = process.env.node-report_EVENTS || 'exception+fatalerror+signal+apicall';


We want to keep the environment variables case (upper), probably without the hyphen.

richardlau · 2017-01-19T18:33:09Z

src/node_report.cc

 *   Verbose mode
 ******************************************************************************/
-unsigned int ProcessNodeReportEvents(const char* args) {
+unsigned int ProcessProcessNode_ReportEvents(const char* args) {


Extra Process. Doesn't match the call in src/module.cc.

richardlau · 2017-01-19T18:37:03Z

I'd like a second opinion on the function name changes in the C++ code -- I'm finding it much harder to read. When I see ProcessNode_ReportCoreSwitch I'm mentally parsing the underscore as a separator so I read it as ProcessNode and ReportCoreSwitch.

sam-github

You don't have to change the js and c++ symbol names, and many of the name changes are syntactically invalid. The env var names don't need changing, either. A number of comments refer to a report as a "NodeReport", which was weird to begin with, just call it a "report", which is better wording, and robust to module name changes. A bunch of other comments unnecessarily name the module, just remove that part of the comment. Any reader of the code surely knows the module the code is part of! Or can find out easily if they are really confused.

sam-github · 2017-01-19T19:33:00Z

README.md

 application.

 ```js
-var nodereport = require('nodereport');
-nodereport.triggerReport();
+var node-report = require('node-report');


this is syntactically invalid javascript, you need to be more discerning in your search and replace. revert it to the original.

sam-github · 2017-01-19T19:33:36Z

README.md

-export NODEREPORT_DIRECTORY=<full path>
-export NODEREPORT_COREDUMP=yes|no
-export NODEREPORT_VERBOSE=yes|no
+export NODE_REPORT_EVENTS=exception+fatalerror+signal+apicall


These don't have to be changed. Its the module name that is punctuated, we don't have to punctuate the env var names

sam-github · 2017-01-19T19:33:59Z

README.md

 ```

 ## Examples

-To see examples of NodeReports generated from these events you can run the
-demonstration applications provided in the nodereport github repository. These are
+To see examples of node-reports generated from these events you can run the


"of reports generated"

sam-github · 2017-01-19T19:34:41Z

README.md

-To see examples of NodeReports generated from these events you can run the
-demonstration applications provided in the nodereport github repository. These are
+To see examples of node-reports generated from these events you can run the
+demonstration applications provided in the node-report github repository. These are


"in the github repository"

sam-github · 2017-01-19T19:35:05Z

README.md

-2. `exception.js` - NodeReport triggered by unhandled exception.
-3. `fatalerror.js` - NodeReport triggered by fatal error on JavaScript heap out of memory.
-4. `loop.js` - looping application, NodeReport triggered using kill `-USR2 <pid>`.
+1. `api.js` - node-report triggered by JavaScript API call.


for all s/node-report/report/

sam-github · 2017-01-19T19:39:24Z

index.js


 const api = require('./api');

-// NODEREPORT_EVENTS env var overrides the defaults
-const options = process.env.NODEREPORT_EVENTS || 'exception+fatalerror+signal+apicall';
+// NODE_REPORT_EVENTS env var overrides the defaults


revert, unnecessary to change this

sam-github · 2017-01-19T19:39:51Z

src/module.cc

@@ -1,7 +1,7 @@
 #include "node_report.h"
 #include <nan.h>

-namespace nodereport {
+namespace node-report {


invalid, revert to original

I think every change in this file pretty much, except maybe some comments, can be reverted as unnecessary (and won't compile).

sam-github · 2017-01-19T19:41:31Z

src/module.cc

@@ -241,26 +241,26 @@ static int StartWatchdogThread(void* (*thread_main)(void* unused)) {
  pthread_sigmask(SIG_SETMASK, &sigmask, nullptr);
  pthread_attr_destroy(&attr);
  if (err != 0) {
-    fprintf(stderr, "nodereport: StartWatchdogThread pthread_create() failed: %s\n", strerror(err));
+    fprintf(stderr, "node-report: StartWatchdogThread pthread_create() failed: %s\n", strerror(err));


this change is one of the few that should stay, because it names the module

sam-github · 2017-01-19T19:41:45Z

src/module.cc

 inline void* ReportSignalThreadMain(void* unused) {
  for (;;) {
    uv_sem_wait(&report_semaphore);
-    if (nodereport_verbose) {
-      fprintf(stdout, "nodereport: signal %s received\n", node::signo_string(report_signal));
+    if (node-report_verbose) {


don't need this line

sam-github · 2017-01-19T19:41:53Z

src/module.cc

-    if (nodereport_verbose) {
-      fprintf(stdout, "nodereport: signal %s received\n", node::signo_string(report_signal));
+    if (node-report_verbose) {
+      fprintf(stdout, "node-report: signal %s received\n", node::signo_string(report_signal));


do need this line. etc.

richardlau · 2017-01-21T00:34:19Z

Superseded by #43.

richardlau mentioned this pull request Jan 12, 2017

nit? s/nodereport/node-report #35

Closed

richardlau suggested changes Jan 12, 2017

View reviewed changes

gdams force-pushed the master branch 2 times, most recently from 7a9cf0d to 9d757a2 Compare January 13, 2017 08:19

chore: rename nodereport to node-report

2bb988e

gdams force-pushed the master branch from 9d757a2 to 2bb988e Compare January 13, 2017 08:25

richardlau reviewed Jan 19, 2017

View reviewed changes

sam-github suggested changes Jan 19, 2017

View reviewed changes

rnchamberlain mentioned this pull request Jan 20, 2017

Rename nodereport module to node-report #43

Closed

richardlau closed this Jan 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: rename nodereport to node-report #37

chore: rename nodereport to node-report #37

gdams commented Jan 12, 2017 •

edited

Loading

rnchamberlain commented Jan 12, 2017 •

edited

Loading

richardlau left a comment

richardlau Jan 12, 2017

richardlau Jan 12, 2017

richardlau Jan 12, 2017

richardlau Jan 12, 2017

richardlau Jan 12, 2017

richardlau Jan 12, 2017

richardlau Jan 19, 2017

richardlau commented Jan 19, 2017

sam-github left a comment

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

sam-github Jan 19, 2017

richardlau commented Jan 21, 2017

chore: rename nodereport to node-report #37

chore: rename nodereport to node-report #37

Conversation

gdams commented Jan 12, 2017 • edited Loading

rnchamberlain commented Jan 12, 2017 • edited Loading

richardlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Jan 19, 2017

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Jan 21, 2017

gdams commented Jan 12, 2017 •

edited

Loading

rnchamberlain commented Jan 12, 2017 •

edited

Loading