fixed bug of method PyString_FromString #670

yagweb · 2018-05-10T10:09:25Z

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.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

den-run-ai · 2018-05-10T14:28:55Z

@yagweb failing in python 2.7:

https://travis-ci.org/pythonnet/pythonnet/jobs/377211034

runtime.cs(1213,56): error CS1503: Argument 1: cannot convert from 'System.IntPtr' to 'string'

codecov · 2018-05-11T00:39:28Z

Codecov Report

Merging #670 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

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

Flag	Coverage Δ
#setup_linux	`69.42% <ø> (ø)`	⬆️
#setup_windows	`76.42% <100%> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
src/runtime/runtime.cs	`91.4% <100%> (+0.03%)`	⬆️
src/runtime/CustomMarshaler.cs	`57.53% <0%> (-12.33%)`	⬇️

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 fb84cd2...050f8cc. Read the comment docs.

den-run-ai · 2018-05-11T13:57:39Z

@yagweb need a fix for appveyor pip update:

microsoft/vscode-python#1107

den-run-ai · 2018-05-11T17:25:06Z

@yagweb this PR still needs a unit test for the failing case before it can be merged. All tests are passing!

den-run-ai · 2018-05-29T16:54:12Z

@yagweb will this PR fix these 3 disabled tests?

pythonnet/src/embed_tests/TestPyString.cs

Line 38 in 6e4cf9d

[Ignore("Ambiguous behavior between PY2/PY3. Needs remapping")]

pythonnet/src/embed_tests/TestPyString.cs

Line 73 in 6e4cf9d

[Ignore("Ambiguous behavior between PY2/PY3. Needs remapping")]

pythonnet/src/embed_tests/TestPyAnsiString.cs

Line 89 in 6e4cf9d

public void TestUnicode()

yagweb · 2018-05-31T17:48:38Z

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

den-run-ai · 2018-06-04T05:57:20Z

@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:
UnicodeDecodeError: 'ascii' codec can't decode byte 0xec in position 0: ordinal not in range(128)
https://travis-ci.org/pythonnet/pythonnet/jobs/387536002#L2111

Appveyor CI:

assert test_unicode_str == text_type(world)
\x1b[1m\x1b[31mE       AssertionError: assert '\uc548\ub155' == '??'\x1b[0m
\x1b[1m\x1b[31mE         - \uc548\ub155\x1b[0m
E         + ??
src\tests\test_conversion.py:544: AssertionError

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?

filmor · 2018-06-04T06:57:16Z

I don't think this can work on Py2 without breaking backwards compatibility, as we identify System.String with str while it would need to be unicode.

den-run-ai · 2018-06-04T07:04:44Z

@filmor agreed, so then skip the last test on PY2?

filmor · 2018-06-04T08:26:10Z

I think so, yes. Put a comment there for posterity :)

filmor · 2018-06-04T08:29:33Z

src/runtime/runtime.cs

@@ -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);


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

@filmor can you file a separate issue for this with a suggested fix? this PR is now ready to merge!

fixed bug of method PyString_FromString

a0f4c33

den-run-ai mentioned this pull request May 10, 2018

str() failure when C# ToString() return unicode character #666

Closed

fixup! fixed bug of method PyString_FromString

e9451db

removed spaces

7aaa644

den-run-ai added 2 commits May 11, 2018 09:00

Update appveyor.yml

083abfc

Update requirements.txt

9c724e3

Merge branch 'master' into bug_of_pystring

b8c6917

den-run-ai added 6 commits June 3, 2018 05:19

Update conversiontest.cs

42999f7

Update test_conversion.py

1f3255a

Update test_conversion.py

36def5d

Update test_conversion.py

084d743

Update test_conversion.py

92ea629

Update test_conversion.py

947cb01

den-run-ai requested review from vmuriart, dmitriyse and filmor June 4, 2018 07:05

filmor approved these changes Jun 4, 2018

View reviewed changes

den-run-ai and others added 3 commits June 11, 2018 23:46

Update test_conversion.py

c5ec6b3

Merge branch 'master' into bug_of_pystring

a3da211

Merge branch 'master' into bug_of_pystring

050f8cc

filmor added this to the 2.4.0 milestone Jul 23, 2018

filmor merged commit b6417ca into pythonnet:master Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed bug of method PyString_FromString #670

fixed bug of method PyString_FromString #670

yagweb commented May 10, 2018

den-run-ai commented May 10, 2018

codecov bot commented May 11, 2018 •

edited

Loading

den-run-ai commented May 11, 2018

den-run-ai commented May 11, 2018

den-run-ai commented May 29, 2018

yagweb commented May 31, 2018 •

edited

Loading

den-run-ai commented Jun 4, 2018

filmor commented Jun 4, 2018

den-run-ai commented Jun 4, 2018

filmor commented Jun 4, 2018

filmor Jun 4, 2018

den-run-ai Jun 12, 2018

fixed bug of method PyString_FromString #670

fixed bug of method PyString_FromString #670

Conversation

yagweb commented May 10, 2018

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

den-run-ai commented May 10, 2018

codecov bot commented May 11, 2018 • edited Loading

Codecov Report

den-run-ai commented May 11, 2018

den-run-ai commented May 11, 2018

den-run-ai commented May 29, 2018

yagweb commented May 31, 2018 • edited Loading

den-run-ai commented Jun 4, 2018

filmor commented Jun 4, 2018

den-run-ai commented Jun 4, 2018

filmor commented Jun 4, 2018

filmor Jun 4, 2018

Choose a reason for hiding this comment

den-run-ai Jun 12, 2018

Choose a reason for hiding this comment

codecov bot commented May 11, 2018 •

edited

Loading

yagweb commented May 31, 2018 •

edited

Loading