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.

Sign up for GitHub

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

Jump to bottom

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.

eugeneo
eugeneo reviewed Feb 1, 2019
View reviewed changes
deps/v8/src/inspector/PRESUBMIT.py Outdated
@@ -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.

sam-github
sam-github reviewed Feb 1, 2019
View reviewed changes
lib/internal/http2/core.js Outdated
@@ -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 •
edited
Loading

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

Resume: https://ci.nodejs.org/job/node-test-commit/25639/

@rneatherway
Copy link
Contributor Author

Great, the CI passed.

@rneatherway @refack
lib: fix a few minor issues flagged by lgtm
106dd1e 
* 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
@rneatherway @addaleax
lib: fix a few minor issues flagged by lgtm
0b302e4 
* 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/

@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
This was referenced Feb 15, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eugeneo eugeneo eugeneo left review comments

@addaleax addaleax addaleax approved these changes

@refack refack refack approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@rneatherway @devsnek @sam-github @richardlau @refack @eugeneo @addaleax @Trott @nodejs-github-bot