Welcome @aug2uag and thanks for the pull request! A more descriptive commit message would be welcome. Maybe something like this?

src: remove unnecessary header file

src/node_stat_watcher.cc imports `env.h` but it is not necessary to do so.

Fixes: https://github.com/nodejs/node/issues/29975

(Bonus points for adding an explanation as to why it's not necessary to import it!)

@joyeecheung

This file uses Environment::GetCurrent which needs env-inl.h, which is transitively included by other headers here. I am not sure removing env.h is an improvement or not.

cc @addaleax @bnoordhuis @danbev any opinions on this?

Thank you @Trott ! I'm sorry I didn't realize the consequences of no comment. I'm very new to contributing and your comment is very helpful to me.

Instead of removing env.h it should probably be changed to include env-inl.h, that's the predominant pattern in our code base.

src/module_wrap.cc and src/node_report_module.cc could also be updated.

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

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

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

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

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

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

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

Getting a clean CI on this will probably require landing #29979 first.

is that an IE render issue? doctool/test-doctool-html.js may have some issues with the HTML or mismatch issue with MD. test-doctool-html.js:100 doesn't close </a></li> and line 65 has breaking HTML:

<h1>
      Sample Markdown with YAML info
      <span>
            <a class="mark" href="#foo_sample_markdown_with_yaml_info" id="foo_sample_markdown_with_yaml_info">#</a>
      </span>
</h1>
<h2>
      Foobar
      <span>
            <a class="mark" href="#foo_foobar" id="foo_foobar">#</a>
      </span>
</h2>
<div class="api_metadata">
      <span>Added in: v1.0.0</span></div>
      <p>Describe <code>Foobar</code> in more detail here.</p>
      <h2>
            Foobar II
            <span>
                  <a class="mark" href="#foo_foobar_ii" id="foo_foobar_ii">#</a>
            </span>
      </h2>
      <div class="api_metadata">
          <details class="changelog">
              <summary>History</summary>
              <table>
                  <tbody>
                      <tr>
                          <th>Version</th>
                          <th>Changes</th>
                      </tr>
                      <tr>
                          <td>v5.3.0, v4.2.0</td>
                          <td>
                              <p><span>Added in: v5.3.0, v4.2.0</span></p>
                          </td>
                      </tr>
                      <tr>
                          <td>v4.2.0</td>
                          <td>
                              <p>The <code>error</code> parameter can now bean arrow function.</p>
                          </td>
                      </tr>
                  </tbody>
              </table>
          </details>
      </div>
      <p>Describe <code>Foobar II</code> in more detail here.<a href="http://man7.org/linux/man-pages/man1/fg.1.html"><code>fg(1)</code></a></p>
      <h2>Deprecated thingy<span><a class="mark" href="#foo_deprecated_thingy" id="foo_deprecated_thingy">#</a></span></h2>
      <div class="api_metadata"><span>Added in: v1.0.0</span><span>Deprecated since: v2.0.0</span></div>
      <p>Describe <code>Deprecated thingy</code> in more detail here.<a href="http://man7.org/linux/man-pages/man1/fg.1p.html"><code>fg(1p)</code></a></p>
      <h2>Something<span><a class="mark" href="#foo_something" id="foo_something">#</a></span></h2>
      <!-- This is not a metadata comment -->
      <p>Describe <code>Something</code> in more detail here. </p>

Apologies, I wasn't intending to push the changes to lib/_stream_writable.js and I don't know how to revert my changes.

@aug2uag The problem likely is that you are opening this pull request against your own master branch. Create a different branch and open a PR against that branch so you can keep your PR changes separate from each other.

the breaking html was necessary in doctest, sorry about that

this PR should be Ok minus the first commit message i was unable to re-amend

@Trott is the CI on this broken from the commit message or something else?

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

@Trott is the CI on this broken from the commit message or something else?

Just the commit message. If you can rebase and fix it, great. But if not, whoever lands this can also fix it.

@Trott update ready

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

@aug2uag Looks like you created a merge commit here rather than rebasing? Our CI doesn’t really deal well with those…

Anna, sorry I just did the same with the other 29988 (i missed your comment here) .. there were the conflicts that I thought needed to be committed-- i didn't mean to cause you trouble.

@addaleax i tried rebasing but getting that changes are up to date, dunno what to do

@aug2uag Are you rebasing against nodejs/node’s master branch or your own? If you have the nodejs/node repository set as the upstream remote, then git fetch upstream && git rebase upstream/master should help… otherwise, idk but it might be causing troubles that you opened a PR from your repo’s own master branch?

Also, somebody else can do the rebase for you if you prefer.

@addaleax that was it thank you, i followed the prompts and ended up pushing again-- i hope the CI and team aren't too upset here

@aug2uag Nobody’s upset, don’t worry about that :) It looks like some other unrelated changes from another PR have slipped in there, though?

@addaleax thanks, also for your great presentations. it's extraordinary to be interacting with you all on this project. i'm here in case there's anything i can do on my part

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

the failure sequential/test-http-server-keep-alive-timeout-slow-client-headers is known as in #22147

while trying to land this, I get patch is empty message. Looking at the PR changes and the state of upstream/master, it cannot be true; does anyone who what to do?

#curl -L https://github.com/nodejs/node/pull/29975.patch | git am --whitespace=fix
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   109    0   109    0     0    188      0 --:--:-- --:--:-- --:--:--   188
100 96865    0 96865    0     0  56496      0 --:--:--  0:00:01 --:--:--  444k
Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@gireeshpunathil it may be artifacts from my misuse of git, the change may be merged from a clean start if it helps

@aug2uag - no worries at all; I don't think that is the case, but I even don't know how to solve it. Let us wait till some one comes to resolve it!

Landed in 5473ceb :)

while trying to land this, I get patch is empty message. Looking at the PR changes and the state of upstream/master, it cannot be true; does anyone who what to do?

You pointed to the URL for the issue, not the PR 🙂

Landed in 5473ceb :)

The entire commit message is src: rebase for 29976 (which is this PR number). That's confusing, or at least I find it confusing? Too late to force push to improve it? I had suggested this:

src: remove unnecessary header file

src/node_stat_watcher.cc imports `env.h` but it is not necessary to do so.

Fixes: https://github.com/nodejs/node/issues/29975

It's a holiday weekend in the U.S. (and the middle of the night in the Americas), and the weekend everywhere else (well, except in two time zones where it's before 2AM on Monday morning), and things are slow so I force-pushed this as the commit message:

src: change header file in node_stat_watcher.cc

change src/node_stat_watcher.cc to import `env-inl.h` instead of
`env.h`.

PR-URL: https://github.com/nodejs/node/pull/29976
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

@Trott thanks for noticing and fixing this!