Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Apply EBCDIC to ASCII conversions only if node wasn't built with -qASCII #138

Merged
merged 1 commit into from
Oct 31, 2019
Merged

Apply EBCDIC to ASCII conversions only if node wasn't built with -qASCII #138

merged 1 commit into from
Oct 31, 2019

Conversation

gabylb
Copy link
Contributor

@gabylb gabylb commented Oct 28, 2019

Checklist

Description of change

  • node-v8 was built in EBCDIC mode, and node-report, built in ASCII, had to convert strings from EBCDIC to ASCII; this causes node-v12 to fail (4) tests as it's built in ASCII. Now we do those conversions only if the current node version was built in EBCDIC.

mhdawson
mhdawson approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@richardlau
Copy link
Member

richardlau commented Oct 31, 2019 •
edited

CI runs:
Node.js 13: https://ci.nodejs.org/job/nodereport-continuous-integration-latest/48/ (✔️)
Node.js 12: https://ci.nodejs.org/job/nodereport-continuous-integration-latest/47/ (✔️)
Node.js 10: https://ci.nodejs.org/job/nodereport-continuous-integration-latest/44/ (✔️)
Node.js 8: https://ci.nodejs.org/job/nodereport-continuous-integration-latest/49/ (✔️)

@richardlau richardlau merged commit e8bf52a into nodejs:master Oct 31, 2019
richardlau added a commit to richardlau/nodereport that referenced this pull request Oct 31, 2019
@richardlau
2.2.9
86c64a5 
 * Apply EBCDIC to ASCII conversions only if node wasn't built with -qASCII (nodejs#138) (Gaby Baghdadi)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richardlau richardlau richardlau approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gabylb @richardlau @mhdawson