- Notifications
You must be signed in to change notification settings - Fork750
fixed bug of method PyString_FromString#670
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@yagweb failing in python 2.7: https://travis-ci.org/pythonnet/pythonnet/jobs/377211034
|
codecovbot commentedMay 11, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #670 +/- ##==========================================- Coverage 77.4% 77.24% -0.16%========================================== Files 63 63 Lines 5589 5590 +1 Branches 892 892 ==========================================- Hits 4326 4318 -8- Misses 970 980 +10+ Partials 293 292 -1
Continue to review full report at Codecov.
|
@yagweb need a fix for appveyor pip update: |
@yagweb this PR still needs a unit test for the failing case before it can be merged. All tests are passing! |
@yagweb will this PR fix these 3 disabled tests?
|
yagweb commentedMay 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@denfromufa No. I also found another bug of PyString/PyAnsiString, the ToString method of a object constructed using PyString(string) don't work with non-ASCII strings. I will add all these into the checklist and dig deeper when I have time. Maybe they are the cause of#662. |
@yagweb i added the unit test based on reported bug by@EcmaXp in#666 but the issue does not look resolved in Python 2.7, works on all Python 3+ versions: Travis CI: Appveyor CI: https://ci.appveyor.com/project/pythonnet/pythonnet/build/master-1182/job/nvwn4ek3eskteg3m#L220 What do you think? Maybe the test should be modified for Python 2.7? |
I don't think this can work on Py2 without breaking backwards compatibility, as we identify |
@filmor agreed, so then skip the last test on PY2? |
I think so, yes. Put a comment there for posterity :) |
| @@ -1193,7 +1193,11 @@ internal static bool PyString_Check(IntPtr ob) | |||
| internal static IntPtr PyString_FromString(string value) | |||
| { | |||
| #if PYTHON3 | |||
| return PyUnicode_FromKindAndData(_UCS, value, value.Length); | |||
| #elif PYTHON2 | |||
| return PyString_FromStringAndSize(value, value.Length); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe check here whether the string is ASCII? I'm not sure where this one is used, but we should probably distinguish the case where we actually want Unicode (and should also return that on Python 2) from the cases that should be ASCII-only on Python 2 (like identifiers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor can you file a separate issue for this with a suggested fix? this PR is now ready to merge!
What does this implement/fix? Explain your changes.
The second parameter of PyString_FromStringAndSize should be filled with the length of the bytes array, not the the length of the raw string.
This method is a very basic method，sufficient tests should be applied.
Does this close any currently open issues?
#666
A unit test method need be added.
Any other comments?
No.
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG