Skip to content

Fixed "print function's "end" option error in CPython" - #92

Closed
Sungmin-Joo wants to merge 2 commits into
go-python:masterfrom
Sungmin-Joo:master
Closed

Fixed "print function's "end" option error in CPython"#92
Sungmin-Joo wants to merge 2 commits into
go-python:masterfrom
Sungmin-Joo:master

Conversation

@Sungmin-Joo

@Sungmin-JooSungmin-Joo commented Sep 27, 2019

Copy link
Copy Markdown
Contributor

The following code writes different string to the stream than in CPython:

print(1,2,3, end="||\n")

I fixed that bug by modifying the code
resolved: #93

The following code writes different string to the stream than in CPython:
print(1,2,3, end="||\n")
The following code writes different string to the stream than in CPython:
print(1,2,3, end="||\n")
I fixed that bug by modifying the code
@codecov-io

codecov-io commented Sep 27, 2019

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.10%. Comparing base (e427daf) to head (0d6c264).

Additional details and impacted files
@@ Coverage Diff @@## master #92 +/- ##
==========================================
+ Coverage 69.09% 69.10% +0.01% 
==========================================
Files 60 60 Lines 10674 10678 +4 ==========================================
+ Hits 7375 7379 +4 
Misses 2781 2781 Partials 518 518 

☔ 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.

@Sungmin-JooSungmin-Joo changed the title Fix "end" option in print functionFixed "print function's "end" option error in CPython"Sep 27, 2019

@corona10corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Sungmin-Joo Please add a unit test which you updated.

@corona10
corona10 requested a review from ncwSeptember 27, 2019 06:00
add "end" test in cpython
@Sungmin-Joo

Copy link
Copy Markdown
ContributorAuthor

@corona10
Is it a proper test?
If not, I will try again tomorrow.

@corona10

Copy link
Copy Markdown
Collaborator

@Sungmin-Joo
You should show the test

AS-IS

>>>print(1,2,3, end="||\n")
1||2||3||

TO-BE

>>>print(1,2,3, end="||\n")
123||

@corona10corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The test is not proper.

Comment threadbuiltin/builtin.go
return nil, err
}

if kwargs["sep"] != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about:

switchsep, ok:=kwargs["sep"]; ok {
casetrue:
sepObj=sepdefault:
sepObj=py.String(" ")
}

print("hello","gpython", end="123\n")
except TypeError as e:
ok = True
assert ok, "TypeError not raised"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
assertok, "TypeError not raised"
assertok, "TypeError not raised when 'sep' is not in the kwargs dict"

that said, I am not sure I understand this test.
do we really want this line to raise an exception? why?

assert ok, "TypeError not raised"

try:
print("hello","gpython", end="123\n")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
print("hello","gpython", end="123\n")
print("hello","gpython", end="123\n")

@corona10

Copy link
Copy Markdown
Collaborator

@Sungmin-Joo@sbinet
Sorry,
I think that we should fix #90 was not the proper approach.

err:=py.ParseTupleAndKeywords(nil, kwargs, "|ssOO:print", kwlist, &sepObj, &endObj, &file, &flush)

I am now closing this PR
Let's fix this issue with proper approach :)

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.

print function's "end" option error in CPython

4 participants

@Sungmin-Joo@codecov-io@corona10@sbinet