Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a few minor issues flagged by lgtm #25873

Merged
merged 1 commit into from
Feb 6, 2019
Merged

fix a few minor issues flagged by lgtm #25873

merged 1 commit into from
Feb 6, 2019

Conversation

rneatherway
Copy link
Contributor

  • Implicit string concatenation caused the inspector PRESUBMIT to
    ignore some files
  • Spurious additional passed argument to afterShutdown
  • Confusing (but correct) regex using A-z character range by
    accident

I'm one of the developers behind https://lgtm.com and I was browsing the nodejs code over there at https://lgtm.com/projects/g/nodejs/node. I found these fairly simple issues that I could fix, hope this is helpful.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Feb 1, 2019
@devsnek
Copy link
Member

devsnek commented Feb 1, 2019

files in the deps folder should be modified by submitting a patch to the relevent upstream project. the other stuff looks fine.

@@ -17,7 +17,7 @@ def _CompileScripts(input_api, output_api):
local_paths = [f.LocalPath() for f in input_api.AffectedFiles()]

compilation_related_files = [
"js_protocol.json"
"js_protocol.json",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is V8 issue. Can you report it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this file was deleted upstream in October last year. I'll back this change out in any case.

@@ -1820,7 +1820,7 @@ class Http2Stream extends Duplex {
req.handle = handle;
const err = handle.shutdown(req);
if (err === 1) // synchronous finish
return afterShutdown.call(req, 0);
return afterShutdown.call(req);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't traced through the C++ side here, but Isuspect that in the async case, .oncomplete is called with an arg, a status. It happens that afterShutdown() doesn't use it ATM, but if it ever did, this sync call here would be wrong for not passing success/0. Maybe it would be better for afterShutdown() to add the arg, and then comment that its not used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github Your assumptions are correct 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can make that change.

@sam-github
Copy link
Contributor

tools/cpplint.py is also periodically merged from upstream. We have to maintain patches, so probably better to have as few as possible. It looks like the regex there is allowing chars by accident that are not syntactically possible in C++, so are caught by compiler. That should likely be upstreamed to https://github.com/cpplint/cpplint/ and we'll get it back next time we sync to upstream.

/cc @refack (last person who synced cpplint)

@richardlau
Copy link
Member

richardlau commented Feb 1, 2019

tools/cpplint.py is also periodically merged from upstream. We have to maintain patches, so probably better to have as few as possible. It looks like the regex there is allowing chars by accident that are not syntactically possible in C++, so are caught by compiler. That should likely be upstreamed to https://github.com/cpplint/cpplint/ and we'll get it back next time we sync to upstream.

/cc @refack (last person who synced cpplint)

The lines being changed in this PR is from one of our floating patches: cbc3dd9#diff-648c2189bda313e553284785c602c91d

@Trott Trott added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels Feb 1, 2019
@rneatherway
Copy link
Contributor Author

The lines being changed in this PR is from one of our floating patches: cbc3dd9#diff-648c2189bda313e553284785c602c91d

Shall I leave this part as is in that case?

@refack
Copy link
Contributor

refack commented Feb 5, 2019

Shall I leave this part as is in that case?

Sure.

@refack
Copy link
Contributor

refack commented Feb 5, 2019

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

@sam-github you copacetic with current state?

@refack
Copy link
Contributor

refack commented Feb 5, 2019

@rneatherway
Copy link
Contributor Author

Great, the CI passed.

* Confusing (but correct) regex using `A-z` character range by
  accident
* Add the status argument to afterShutdown

PR-URL: #25873
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@refack refack merged commit 106dd1e into nodejs:master Feb 6, 2019
@refack
Copy link
Contributor

refack commented Feb 6, 2019

@rneatherway thanks for the contribution!
Congratulations on getting promoted by GitHub from
image
to
image
Hope to see you contribute more in the future 🎉

addaleax pushed a commit that referenced this pull request Feb 6, 2019
* Confusing (but correct) regex using `A-z` character range by
  accident
* Add the status argument to afterShutdown

PR-URL: #25873
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@rneatherway rneatherway deleted the lgtm-fixes branch February 7, 2019 10:42
@rneatherway
Copy link
Contributor Author

Thanks for merging :-)

If you'd like to know if these kinds of issues pop up on PRs in the future you can enable our code review integration at https://lgtm.com/projects/g/nodejs/node/ci/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants