cleanup of TODO comments in configure and removal of Makefile.build #5080

ojss · 2016-02-04T19:03:02Z

This pull request is for issue #4607
Particularly issues : 2,3,4 mentioned in #4607 are handled.

Makefile.build is removed
A comment in configure has been removed.
A help message has been added in configure under --systemtap-includes

bnoordhuis · 2016-02-04T21:06:12Z

configure

@@ -1050,7 +1050,7 @@ def configure_intl(o):
  if not icu_ver_major:
    print ' Could not read U_ICU_VERSION_SHORT version from %s' % uvernum_h
    sys.exit(1)
-  icu_endianness = sys.byteorder[0];  # TODO(srl295): EBCDIC should be 'e'
+  icu_endianness = sys.byteorder[0];


Please undo this change, or move it to a separate commit. Rule of thumb: one change per commit, no more, no less.

bnoordhuis · 2016-02-04T21:06:36Z

Can you make sure that the commit logs follow the guidelines from CONTRIBUTING.md? Thanks.

ojss · 2016-02-05T05:15:27Z

I have reverted to a previous state and made the changes in separate commits.
Is that fine? I have also followed the commit log guidelines this time.

ojss · 2016-02-06T17:03:21Z

May I know of the pull request has been reviewed?

ojss · 2016-02-14T05:04:49Z

May I please know if this PR has been reviewed?

Trott · 2016-02-14T22:09:20Z

Hi, @ojss! Can you please either put the three changes here in three separate PRs?

If you don't want to do that, an acceptable alternative might be to leave the changes in a single PR but have separate commits for each of the three changes. I see you have six commits in this PR currently. If you can squash them to three commits (one for each change) and then force push that, then that might be OK.

The removing of Makefile.build looks good to me. If you just want to do only one of the change to get comfortable with the process, I'd do that one, because it's so straightforward.

removed Makefile.build, as it is not really used by anyone. Refer to issue nodejs#4607

removed a redundant TODO in configure: "# TODO(srl295): EBCDIC should be 'e'" as there is no plan to support EBCDIC systems any time soon.

added a help message for --systemtap-includes optparse.SUPPRESS_HELP was replaced by help message and the TODO comment was removed

ojss · 2016-02-15T13:41:32Z

Hello, @Trott! I have removed the pointless commits and force pushed the branch. So there are only the three meaningful commits remaining. How does it look?

bnoordhuis · 2016-02-15T17:31:19Z

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/1665/

Apropos the commit log, we normally use present tense (e.g. 'add' instead of 'added') but that's something we can fix up when it lands.

ojss · 2016-02-15T17:38:46Z

@bnoordhuis, yeah I have seen a lot of projects do that, but it feels really awkward to make a commit log like that.
Is the CI link only for people with read/write access?

bnoordhuis · 2016-02-15T17:47:13Z

It's the style we use and consistency is paramount. The CI is currently private because of a Jenkins issue. Sorry, should have mentioned that, I forgot.

ojss · 2016-02-16T06:55:47Z

@bnoordhuis I understand.
Will you be amending the commit logs to fix the tense?

Trott · 2016-02-16T20:56:41Z

Unless someone raises concerns or objections pretty much right away, I'll run CI again and (if successful) land this after amending the commit logs.

Trott · 2016-02-16T20:57:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1681/

Trott · 2016-02-16T21:18:31Z

Known-flaky failure on SmartOS, but just in case, re-running: https://ci.nodejs.org/job/node-test-commit/2257/

Remove Makefile.build, as it is not really used by anyone. Refs: #4607 PR-URL: #5080 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

Remove a redundant TODO in configure: "# TODO(srl295): EBCDIC should be 'e'" as there is no plan to support EBCDIC systems any time soon. Refs: #4607 PR-URL: #5080 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

Add a help message for --systemtap-includes optparse.SUPPRESS_HELP was replaced by help message and the TODO comment was removed Refs: #4607 PR-URL: #5080 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2016-02-16T21:46:02Z

CI is green. Landed in 8c22c70, 59c5ebf, and cf9bdcf.

Thanks for the work and patience, @ojss!

ojss · 2016-02-17T03:31:29Z

@bnoordhuis and @Trott thanks for all the help and guidance! 😃
I hope to contribute and learn more in the future!
Any place I should start from? Is there some internal documentation that I should look at?

Trott · 2016-02-17T03:56:20Z

@ojss Like everyone, I'm biased towards what works for me and what interests me, and in the case of Node.js, that would be tests.

In no particular order, I would say easy points of entry are:

picking things with good-first-contribution and/or help-wanted labels
improving docs for whatever area you are familiar with or highly interested in learning more about
fixing flaky tests
whatever excites and interests you or is presenting you with an issue on some project you'd like to move forward

Trott · 2016-02-17T04:16:28Z

@ojss Oh, and there are probably some good things to look at in nodejs/inclusivity#96 and the issues it links to (especially the issue linked by Fishrock123).

Remove Makefile.build, as it is not really used by anyone. Refs: #4607 PR-URL: #5080 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

Remove a redundant TODO in configure: "# TODO(srl295): EBCDIC should be 'e'" as there is no plan to support EBCDIC systems any time soon. Refs: #4607 PR-URL: #5080 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

Add a help message for --systemtap-includes optparse.SUPPRESS_HELP was replaced by help message and the TODO comment was removed Refs: #4607 PR-URL: #5080 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>

ojss · 2016-02-19T09:18:22Z

@Trott, thanks I will take a look at that! I hope to talk more on the IRC.