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

tools: prefer filter to remove empty strings #23727

Merged
merged 1 commit into from
Oct 18, 2018
Merged

tools: prefer filter to remove empty strings #23727

merged 1 commit into from
Oct 18, 2018

Conversation

thefourtheye
Copy link
Contributor

Ref: #23585 (comment)

Python's list.remove will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of list.remove with a filter which
solves both of the above mentioned problems.

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

cc @refack @rvagg @nodejs/python

@nodejs-github-bot
Copy link
Collaborator

@thefourtheye build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1280/pipeline

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Oct 18, 2018
@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 18, 2018
@refack
Copy link
Contributor

refack commented Oct 18, 2018

I would even suggest to fast-track this since it solves a regression.
(Please 👍 if you approve)

@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Oct 18, 2018
@refack refack self-assigned this Oct 18, 2018
@refack
Copy link
Contributor

refack commented Oct 18, 2018

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

@thefourtheye @refack
tools: prefer filter to remove empty strings
1d152b6 
Ref: nodejs#23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: nodejs#23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack
Copy link
Contributor

refack commented Oct 18, 2018

Fast-tracked with two 👍 as 1d152b6

@refack refack merged commit 1d152b6 into nodejs:master Oct 18, 2018
@thefourtheye thefourtheye deleted the fix-test-py branch October 19, 2018 05:04
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@thefourtheye @addaleax
tools: prefer filter to remove empty strings
78d4167 
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack refack removed their assignment Oct 20, 2018
jasnell pushed a commit that referenced this pull request Oct 21, 2018
@thefourtheye @jasnell
tools: prefer filter to remove empty strings
77b3666 
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@thefourtheye @MylesBorins
tools: prefer filter to remove empty strings
ac83a14 
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@thefourtheye @rvagg
tools: prefer filter to remove empty strings
72b9d15 
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@thefourtheye @MylesBorins
tools: prefer filter to remove empty strings
ca1c42f 
Ref: #23585 (comment)

Python's `list.remove` will throw if the element is not found and also
it removes only the first occurrence.

This patch replaces the use of `list.remove` with a `filter` which
solves both of the above mentioned problems.

PR-URL: #23727
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@mmarchini mmarchini mmarchini approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@rvagg rvagg rvagg approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@thefourtheye @nodejs-github-bot @refack @jasnell @rvagg @lpinca @cjihrig @mmarchini @richardlau