Skip to content

Conversation

@corona10
Copy link
Collaborator

@corona10 corona10 commented Jul 26, 2019

Implement builtin_ascii

@corona10 corona10 added the WIP label Jul 26, 2019
@corona10 corona10 changed the title [WIP] builtin: Implement builtin_ascii builtin: Implement builtin_ascii Jul 26, 2019
@corona10 corona10 removed the WIP label Jul 26, 2019
@corona10 corona10 requested a review from ncw July 26, 2019 15:07
@corona10
Copy link
Collaborator Author

@ncw
Please take a look!

@corona10 corona10 requested a review from a team July 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 lines Patch % Lines
builtin/builtin.go 75.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

@ncw ncw 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
Collaborator Author

@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
Collaborator Author

I've found this code does not work well.

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

@corona10 corona10 changed the title builtin: Implement builtin_ascii [WIP] builtin: Implement builtin_ascii Jul 28, 2019
@corona10 corona10 changed the title [WIP] builtin: Implement builtin_ascii builtin: Implement builtin_ascii Jul 28, 2019
@corona10 corona10 changed the title builtin: Implement builtin_ascii [WIP] builtin: Implement builtin_ascii Jul 28, 2019
@corona10 corona10 added the WIP label Jul 28, 2019
@corona10 corona10 changed the title [WIP] builtin: Implement builtin_ascii builtin: Implement builtin_ascii Jul 31, 2019
@corona10
Copy link
Collaborator Author

@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!

func stringEscape(a py.String) string {
    s := string(a)
    var out bytes.Buffer
    for _, c := range s {
        switch {
        case c < 0x20:
            switch c {
            case '\t':
                out.WriteString(`\t`)
            case '\n':
                out.WriteString(`\n`)
            case '\r':
                out.WriteString(`\r`)
            default:
                out.WriteRune(c)
            }
        case c < 0x100:
            out.WriteRune(c)
        case c < 0x10000:
            fmt.Fprintf(&out, "\\u%04x", c)
        default:
            fmt.Fprintf(&out, "\\U%08x", c)
        }
    }
    return out.String()
}

@corona10 corona10 removed the WIP label Jul 31, 2019
@ncw
Copy link
Collaborator

ncw commented Aug 4, 2019

LGTM - thank you - please merge :-)

@corona10 corona10 merged commit fff175c into go-python:master Aug 4, 2019
@corona10 corona10 deleted the ascii branch August 4, 2019 11:53
Sign up for free to 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