Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

convert sbcs codec and some tests #255

Merged
merged 1 commit into from
Jul 17, 2020

Conversation

gyzerok
Copy link
Contributor

@gyzerok gyzerok commented Jul 16, 2020

Here is the update for SBCS.

My only problem is - I am not sure how to convert sbcs-test.js because it uses iconv and I am not sure what to do with some usages of Buffer there. Maybe it'd be better for you to take a look at this suite?

Otherwise I think everything is there.

@gyzerok
Copy link
Contributor Author

gyzerok commented Jul 16, 2020

The test failures seem to be from the stuff in master and are not related to my PR

@ashtuchkin ashtuchkin force-pushed the master branch 2 times, most recently from 84ee650 to 9aa082f Compare July 16, 2020 08:07
Copy link
Owner

@ashtuchkin ashtuchkin left a comment

Choose a reason for hiding this comment

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

Looks great!

newBuf[idx2] = decodeBuf[idx1];
newBuf[idx2+1] = decodeBuf[idx1+1];
for (let i = 0; i < str.length; i++)
bytes[i] = this.encodeBuf[str.charCodeAt(i)];
Copy link
Owner

Choose a reason for hiding this comment

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

love how concise that is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The funny part is, I was expecting the the whole migration will be more like a manual labor. Just changing calls to Buffer.from and Buffer.toString to our custom functions. So it took me some time to figure out how change the code. But in the end was interesting to dig a little bit deeper into encodings business.

@ashtuchkin
Copy link
Owner

Looks like the master is green now, so could you rebase and I'll merge tomorrow.

@gyzerok
Copy link
Contributor Author

gyzerok commented Jul 16, 2020

@ashtuchkin rebased on master. All green now.

@ashtuchkin ashtuchkin merged commit 228af9c into ashtuchkin:master Jul 17, 2020
@ashtuchkin
Copy link
Owner

Thank you!
Note I've added eslint/prettier integration on the latest master, so be sure to rebase if you decide to convert something else.

@gyzerok gyzerok deleted the convert-sbcs branch July 18, 2020 03:59
@gyzerok
Copy link
Contributor Author

gyzerok commented Jul 18, 2020

@ashtuchkin this is great news! I was missing prettier in the workflow very much :)

Which codec do you think would be better for me to take? Probably in my case better == simpler to migrate.

@ashtuchkin
Copy link
Owner

ashtuchkin commented Jul 19, 2020 via email

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants