-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
build: remove duplicated code #13482
Conversation
Makefile
Outdated
@@ -866,15 +863,17 @@ bench: bench-net bench-http bench-fs bench-tls | |||
|
|||
bench-ci: bench | |||
|
|||
LINT_TARGETS = benchmark doc lib test tools |
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.
Nit: maybe JSLINT_TARGETS since these are the directories for JavaScript linting but there's a separate C++ linting process that lints stuff elsewhere?
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.
@Trott completely agree. thank you :) change is done.
Makefile contains copy-pasted code in some targets and this commit aims to remove it.
32b7dba
to
26082c6
Compare
@nodejs/build |
Could someone re-run the CI job please. Fails seems unrelated. AIX error is already have filled issue here - #13577 |
As far as I remember it was issue on CI, something with disk capacity if I'm not mistaken. It should be fixed. Could please some one re-run the CI again |
Hm... every CI build here is either failing or can't be completed due to my karma I guess 🤣 Could you please help me to understand why CI check is in |
Let's try again... CI: https://ci.nodejs.org/job/node-test-pull-request/9331/ |
New CI (GitHub integration is broken at the moment, that's why GitHub still says there are outstanding tests): https://ci.nodejs.org/job/node-test-pull-request/9684/ |
Landed in 2710616, thank you! 🎉 |
Makefile contains copy-pasted code in some targets and this commit aims to remove it. PR-URL: #13482 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
🎉 thank you! |
@krydos thank you for updating your avatar 😄. Personally I find it helps to associate a face with the online persona. |
@refack, 😄. Just got suggestion from another person that people likes real faces on avatars more than something else. I'm glad I got feedback from you immediately, it proves the theory :) |
Makefile contains copy-pasted code in some targets and this commit aims to remove it. PR-URL: nodejs/node#13482 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Confirmed fix - https://nodejs.org/download/nightly/v9.0.0-nightly20170824abced13e29/docs/api/ |
Makefile contains copy-pasted code in some targets and this commit aims to remove it. PR-URL: #13482 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
Makefile contains copy-pasted code in some targets and this commit aims to remove it. PR-URL: #13482 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kunal Pathak <kunal.pathak@microsoft.com>
This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported |
Hi :)
I noticed that
Makefile
contains duplicated code in some places and in this PR I tired to move this code to variable where possible.Here is exactly what I did:
jslint
andjslint-ci
had (and probably should have) the same list of directories to lint. This list was copy/pasted from one target to another. Just moved it to separated variable.targets that generates
json
andhtml
docs. There was pretty big bash script with very small difference for these targets. Again, the command was just moved to separated variable.There is no issue filled for this, but I hope the change can be useful.
Thank you.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build