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

tools: prefer filter to remove empty strings #23727

Merged
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 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

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 <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
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 <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack removed their assignment Oct 20, 2018
jasnell pushed a commit that referenced this pull request Oct 21, 2018
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 <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
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 <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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 <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
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 <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants