Skip to content

Conversation

@corona10
Copy link
Collaborator

@corona10corona10 commented Jul 26, 2019

Implement builtin_ascii

@corona10corona10 changed the title [WIP] builtin: Implement builtin_asciibuiltin: Implement builtin_asciiJul 26, 2019
@corona10corona10 removed the WIP label Jul 26, 2019
@corona10corona10 requested a review from ncwJuly 26, 2019 15:07
@corona10
Copy link
CollaboratorAuthor

@ncw
Please take a look!

@corona10corona10 requested a review from a teamJuly 26, 2019 15:09
@codecov-io
Copy link

codecov-io commented Jul 26, 2019

Codecov Report

❌ Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.63%. Comparing base (c5b8c68) to head (a3dfc79).

Files with missing linesPatch %Lines
builtin/builtin.go75.00%1 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## master #66 +/- ## ========================================== + Coverage 68.45% 68.63% +0.18%  ========================================== Files 59 59 Lines 10495 10512 +17 ========================================== + Hits 7184 7215 +31 + Misses 2804 2789 -15 - Partials 507 508 +1 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@ncwncw left a comment

Choose a reason for hiding this comment

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

See inline for adapting an existing function to do this

@corona10
Copy link
CollaboratorAuthor

@ncw
Thanks for the kind review.
I 've added additional tests.
But I didn't reflect some of your reviews that I 've not understood yet.
I left comments on that!
Please take a look.

@corona10
Copy link
CollaboratorAuthor

I've found this code does not work well.

>>>ascii('\U+10001') "'\\U00010001'"

@corona10corona10 changed the title builtin: Implement builtin_ascii[WIP] builtin: Implement builtin_asciiJul 28, 2019
@corona10corona10 changed the title [WIP] builtin: Implement builtin_asciibuiltin: Implement builtin_asciiJul 28, 2019
@corona10corona10 changed the title builtin: Implement builtin_ascii[WIP] builtin: Implement builtin_asciiJul 28, 2019
@corona10corona10 changed the title [WIP] builtin: Implement builtin_asciibuiltin: Implement builtin_asciiJul 31, 2019
@corona10
Copy link
CollaboratorAuthor

@ncw
I've added StringEscape which can be used for repr or ascii.
Note that StringEscape which need to ascii is below code.

If there is something I missed. Please let me know!

funcstringEscape(a py.String) string{s:=string(a) varout bytes.Bufferfor_, c:=ranges{switch{casec<0x20: switchc{case'\t': out.WriteString(`\t`) case'\n': out.WriteString(`\n`) case'\r': out.WriteString(`\r`) default: out.WriteRune(c) } casec<0x100: out.WriteRune(c) casec<0x10000: fmt.Fprintf(&out, "\\u%04x", c) default: fmt.Fprintf(&out, "\\U%08x", c) } } returnout.String() }

@corona10corona10 removed the WIP label Jul 31, 2019
ncw
ncw approved these changes Aug 4, 2019
@ncw
Copy link
Collaborator

ncw commented Aug 4, 2019

LGTM - thank you - please merge :-)

@corona10corona10 merged commit fff175c into go-python:masterAug 4, 2019
@corona10corona10 deleted the ascii branch August 4, 2019 11:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@corona10@codecov-io@ncw