-
-
Notifications
You must be signed in to change notification settings - Fork 36
Rename nodereport module to node-report #43
Conversation
Provides code, documentation, test and demo changes required for the migration of the nodereport project to node-report. The main changes are in the build, tests and demos that use the module name, and in the name of the report file. Remaining changes are mostly in comments. Internal variable and macro names are not changed.
| 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 reports generated from these events you can run the | ||
| demonstration applications provided in the node-report github repository. These are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could hyperlink to github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a wider issue, that the same README.md gets used on the npm site and on the github repo, but the information I'd like to have on those two sites is different (the audience is different). So on github I'd like to document the build process, tests and demos, but that's not relevant to the npm user (e.g. they don't have the tests).
Maybe I could put the extra developer info on a page on the github repo wiki, and have a link to that from the README. Or add additional .md files in a docs directory. I wonder what other projects do for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most projects put user-focussed info at top of readme, and other info at bottom (and ignore that its not really intended for the npmjs.org audience). I rarely read the info on npmjs.org past the first screen, I usually just jump directly to github even to read using the link at the top-right. You could also rip the dev-focussed stuff into a CONTRIBUTING.md in the top-level, which would get it off npmjs.org. And then link to it from the README, or not? I wouldn't put dev info in a wiki, its too far from the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll add a dev section at the bottom of the readme, under a separate PR, thanks.
richardlau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add node-report*.txt to .gitignore. I'd add rather than replace so any old style reports are still ignored. Otherwise LGTM.
src/module.cc
Outdated
| nodereport_events = ProcessNodeReportEvents(*parameter); | ||
|
|
||
| // If NodeReport newly requested for fatalerror, set up the V8 call-back | ||
| // If report newly requested for fatalerror, set up the V8 call-back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're editing -- Should this be "callback" (without the hyphen)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
mhdawson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolved conflict in test/common.js
|
This PR got in a mess and would not rebase, closing and raising a new one :-( |
Provides code, documentation, test and demo changes required for the migration of the nodereport project to node-report. The main changes are in the build, tests and demos that use the module name, and in the name of the report file. Remaining changes are mostly in comments and docs. Internal variable and macro names are not changed.
See issue #35. This PR supercedes PR #37
Builds and tests pass on Linux and Windows. I think we should rename the repo before this lands.