Skip to content

builtin: Implement builtin_ascii #66

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

Merged
merged 1 commit into from
Aug 4, 2019
Merged

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

Merging #66 into master will increase coverage by 0.01%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   68.61%   68.63%   +0.01%     
==========================================
  Files          59       59              
  Lines       10499    10512      +13     
==========================================
+ Hits         7204     7215      +11     
- Misses       2788     2789       +1     
- Partials      507      508       +1
Impacted Files Coverage Δ
py/string.go 87.17% <100%> (+0.23%) ⬆️
builtin/builtin.go 79.55% <75%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd0985b...a3dfc79. Read the comment docs.

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