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 #31650 right use of string and bytes objects #31659

Closed
wants to merge 5 commits into from

Conversation

bioinfornatics
Copy link

@bioinfornatics bioinfornatics commented Feb 6, 2020

Fixes: #31650

Checklist
  • [ x] make -j4 test (UNIX), or vcbuild test (Windows) passes

  • [x ] commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Feb 6, 2020
@addaleax addaleax requested a review from cclauss February 6, 2020 12:41
@addaleax addaleax added the python PRs and issues that require attention from people who are familiar with Python. label Feb 6, 2020
@Trott
Copy link
Member

Trott commented Feb 6, 2020

@nodejs/python

@richardlau richardlau linked an issue Feb 6, 2020 that may be closed by this pull request
tools/icu/icutrim.py Outdated Show resolved Hide resolved
tools/icu/icutrim.py Outdated Show resolved Hide resolved
tools/icu/icutrim.py Outdated Show resolved Hide resolved
tools/icu/icutrim.py Outdated Show resolved Hide resolved
tools/icu/icutrim.py Outdated Show resolved Hide resolved
@bioinfornatics bioinfornatics changed the title fix #31650i right use of string and bytes objects fix #31650 right use of string and bytes objects Mar 2, 2020
@bioinfornatics
Copy link
Author

Thanks everybody for your help! 👍

@cclauss
Copy link
Contributor

cclauss commented Mar 4, 2020

@nodejs/python

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

RSLGTM. Note to landers... the PR will need to be squashed and the commit message rewritten to match the commit guidelines: https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

@bioinfornatics you can rebase, squash, fix the message, and force-push the branch yourself, or if you aren't comfortable with that, it can be done during landing.

@nodejs-github-bot
Copy link
Collaborator

@sgallagher
Copy link
Contributor

LGTM. I'm currently carrying this patch in Fedora to allow building on systems with no python2.

jasnell pushed a commit that referenced this pull request Mar 18, 2020
PR-URL: #31659
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Mar 18, 2020

Landed in a squashed commit 5f0af2a

@jasnell jasnell closed this Mar 18, 2020
@cclauss
Copy link
Contributor

cclauss commented Mar 18, 2020

@bioinfornatics and team, thanks for your perseverance on this one... It is good that it has landed.

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

post merge LGTM… Will have to figure out how to rebase #32347 though.

MylesBorins pushed a commit that referenced this pull request Mar 19, 2020
PR-URL: #31659
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
PR-URL: #31659
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Apr 22, 2020
PR-URL: #31659
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@srl295 srl295 mentioned this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build nodejs 13.7.0 fail while invoking "icutrim.py"