Prevent the 'u' prefix on keys #224

jmwri · 2016-09-28T13:07:56Z

Fixes #218

coveralls · 2016-09-28T13:10:49Z

Coverage decreased (-3.2%) to 91.485% when pulling 902e715 on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T13:22:16Z

Coverage decreased (-0.2%) to 94.463% when pulling 6069824 on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T13:22:16Z

Coverage decreased (-0.2%) to 94.463% when pulling 6069824 on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T13:22:16Z

Coverage decreased (-0.2%) to 94.463% when pulling 6069824 on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T13:22:16Z

Coverage decreased (-0.2%) to 94.463% when pulling 6069824 on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T13:22:16Z

Coverage decreased (-0.2%) to 94.463% when pulling 6069824 on jmwri:master into 987fd98 on alecthomas:master.

tusharmakkar08 · 2016-09-28T13:23:03Z

Hey @jmwri

Travis build is failing. Please update the PR.

Thanks

coveralls · 2016-09-28T13:33:27Z

Coverage increased (+0.05%) to 94.743% when pulling 5a9e2ee on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T13:53:53Z

Coverage increased (+0.05%) to 94.743% when pulling 4d58148 on jmwri:master into 987fd98 on alecthomas:master.

jmwri · 2016-09-28T13:54:39Z

@tusharmakkar08 Sorry about that, had some issues with 3.2. I've added six to install_requires.

tusharmakkar08

Please fix the comments.

tusharmakkar08 · 2016-09-28T13:58:12Z

voluptuous/encoding.py

@@ -0,0 +1,5 @@
+def to_utf8_py2(data):


Can we move this to util.py?

I'm having some trouble with this. When I move the function I'm getting lots of doctest errors.

What errors are you getting ? Is it due to circular dependency?

Lots of AttributeErrors and NameErrors:

File "/Users/jim/Projects/voluptuous/README.md", line 578, in README.md Failed example: schema([6]) Exception raised: Traceback (most recent call last): File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/doctest.py", line 1315, in __run compileflags, 1) in test.globs File "<doctest README.md[106]>", line 1, in <module> schema([6]) NameError: name 'schema' is not defined

Seems to be caused by the import at the top of schema_builder.py. If I move the import to the functions where they are used then it's fine.

Having checked util.py it is importing schema_builder.py so yes, it looks like a circular import.

tusharmakkar08 · 2016-09-28T13:58:27Z

voluptuous/schema_builder.py

@@ -260,12 +260,14 @@ def _compile_mapping(self, schema, invalid_msg=None):
        candidates = list(_iterate_mapping_candidates(_compiled_schema))

        def validate_mapping(path, iterable, out):
+            from .encoding import to_utf8_py2


Please move the imports to top.

tusharmakkar08 · 2016-09-28T13:58:36Z

voluptuous/schema_builder.py

@@ -809,6 +811,8 @@ class Marker(object):
    """Mark nodes for special treatment."""

    def __init__(self, schema_, msg=None):
+        from .encoding import to_utf8_py2


Please move the imports to top

tusharmakkar08 · 2016-09-28T13:58:53Z

voluptuous/tests/tests.py

+
+def test_unicode_key_is_converted_to_utf8_when_in_marker():
+    """Verify that when using unicode key the 'u' prefix is not thrown in the exception"""
+    import six


Move the imports to top

tusharmakkar08 · 2016-09-28T13:59:01Z

voluptuous/tests/tests.py

+
+
+def test_unicode_key_is_converted_to_utf8_when_plain_text():
+    import six


Move the imports to top

tusharmakkar08 · 2016-09-28T13:59:06Z

voluptuous/tests/tests.py

+
+
+def test_to_utf8_py2():
+    import six


Move the imports to top

tusharmakkar08 · 2016-09-28T14:00:33Z

voluptuous/encoding.py

@@ -0,0 +1,5 @@
+def to_utf8_py2(data):
+    import sys


Move this import to top

tusharmakkar08 · 2016-09-28T14:00:49Z

setup.py

@@ -50,5 +50,6 @@
    ],
    install_requires=[
        'setuptools >= 0.6b1',
+        'six >= 1.10'


Add it in requirements.txt

I don't have a requirements.txt from the fork?

I am making one.

coveralls · 2016-09-28T14:55:07Z

Coverage increased (+0.05%) to 94.737% when pulling cdaac81 on jmwri:master into 987fd98 on alecthomas:master.

coveralls · 2016-09-28T14:58:59Z

Coverage increased (+0.05%) to 94.743% when pulling 8762e73 on jmwri:master into 987fd98 on alecthomas:master.

tusharmakkar08

Please reply to the comments and fix the suggestions. Also after that it would be great if you can squash the commits into one.

tusharmakkar08 · 2016-09-29T04:57:49Z

requirements.txt

@@ -0,0 +1,2 @@
+nose==1.3.7


You will have to update setup.py also for adding requirements.txt to install_requires.

We can now remove requirements.txt since this is not a dependency on voluptuous but a dependency on tests.

tusharmakkar08 · 2016-09-29T04:58:33Z

voluptuous/encoding.py

@@ -0,0 +1,5 @@
+def to_utf8_py2(data):


What errors are you getting ? Is it due to circular dependency?

tusharmakkar08 · 2016-09-29T05:00:02Z

voluptuous/tests/tests.py

@@ -8,6 +8,8 @@
    validate, ExactSequence, Equal, Unordered
 )
 from voluptuous.humanize import humanize_error
+from voluptuous.encoding import to_utf8_py2
+import six


Can we get this done without importing six? Because Ideally we don't want voluptuous to have external dependencies. Check this

I've had to make separate tests for py2 & 3 due to checking for unicode or str objects.

tusharmakkar08 · 2016-09-29T09:01:59Z

@jmwri : There are few conflicts. Please resolve the conflicts and update the PR.

coveralls · 2016-09-29T09:51:19Z

Coverage increased (+0.08%) to 94.815% when pulling 7e05824 on jmwri:master into 8f5a41b on alecthomas:master.

tusharmakkar08

Please fix the failing tests and the comments.

tusharmakkar08 · 2016-09-29T09:55:55Z

requirements.txt

@@ -0,0 +1,2 @@
+nose==1.3.7


We can now remove requirements.txt since this is not a dependency on voluptuous but a dependency on tests.

tusharmakkar08 · 2016-09-29T09:56:13Z

setup.py

@@ -22,6 +22,13 @@

 description = long_description.splitlines()[0].strip()

+try:


Please revert it back to original setup.py

coveralls · 2016-09-29T10:10:05Z

Coverage increased (+0.1%) to 94.842% when pulling 6482aa6 on jmwri:master into 8f5a41b on alecthomas:master.

coveralls · 2016-09-29T10:12:40Z

Coverage increased (+0.1%) to 94.842% when pulling 6266c9e on jmwri:master into 8f5a41b on alecthomas:master.

coveralls · 2016-09-29T10:12:41Z

Coverage increased (+0.1%) to 94.842% when pulling 6266c9e on jmwri:master into 8f5a41b on alecthomas:master.

tusharmakkar08

Please fix the comments and squash the commits

tusharmakkar08 · 2016-09-29T11:14:27Z

setup.py

@@ -22,7 +22,6 @@

 description = long_description.splitlines()[0].strip()

-


Please revert this change as well

tusharmakkar08 · 2016-09-29T11:16:30Z

voluptuous/util.py

+    return data
+
+
+if sys.version_info < (3,):


Why not doing something like:

def u(x): if sys.version_info < (3, ): return unicode(x) else: return x

# Conflicts: # voluptuous/tests/tests.py

coveralls · 2016-09-29T11:46:36Z

Coverage increased (+0.1%) to 94.837% when pulling 0a70407 on jmwri:master into 8f5a41b on alecthomas:master.

tusharmakkar08 · 2016-09-29T12:25:13Z

@jmwri : Merged 🍰

jmwri · 2016-09-29T12:44:32Z

Yay!

tusharmakkar08 requested changes Sep 28, 2016

View reviewed changes

tusharmakkar08 requested changes Sep 29, 2016

View reviewed changes

tusharmakkar08 added the in progress label Sep 29, 2016

tusharmakkar08 requested changes Sep 29, 2016

View reviewed changes

jmwri added 2 commits September 29, 2016 12:34

Prevent the 'u' prefix on keys

5e3bf14

Merge commit '8f5a41b20c80b3ccc82eb60c7c7668564842008d'

0a70407

# Conflicts: # voluptuous/tests/tests.py

tusharmakkar08 approved these changes Sep 29, 2016

View reviewed changes

tusharmakkar08 merged commit 6308d04 into alecthomas:master Sep 29, 2016

pyup-bot mentioned this pull request Apr 12, 2017

Update voluptuous to 0.10.2 bossjones/scarlett_os#48

Closed

pyup-bot mentioned this pull request Feb 15, 2018

Update voluptuous to 0.11.1 sohonetlabs/generate-ipdatabase#27

Closed



		def test_unicode_key_is_converted_to_utf8_when_plain_text():
		import six

		@@ -22,6 +22,13 @@

		description = long_description.splitlines()[0].strip()

		try:

		@@ -22,7 +22,6 @@

		description = long_description.splitlines()[0].strip()

Prevent the 'u' prefix on keys #224

Prevent the 'u' prefix on keys #224

Conversation

jmwri commented Sep 28, 2016

coveralls commented Sep 28, 2016 • edited Loading

coveralls commented Sep 28, 2016

coveralls commented Sep 28, 2016

coveralls commented Sep 28, 2016

coveralls commented Sep 28, 2016

coveralls commented Sep 28, 2016

tusharmakkar08 commented Sep 28, 2016

coveralls commented Sep 28, 2016 • edited Loading

coveralls commented Sep 28, 2016 • edited Loading

jmwri commented Sep 28, 2016

tusharmakkar08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 28, 2016 • edited Loading

coveralls commented Sep 28, 2016 • edited Loading

tusharmakkar08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tusharmakkar08 Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tusharmakkar08 commented Sep 29, 2016

coveralls commented Sep 29, 2016 • edited Loading

tusharmakkar08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 29, 2016 • edited Loading

coveralls commented Sep 29, 2016

coveralls commented Sep 29, 2016 • edited Loading

tusharmakkar08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 29, 2016 • edited Loading

tusharmakkar08 commented Sep 29, 2016

jmwri commented Sep 29, 2016

coveralls commented Sep 28, 2016 •

edited

Loading

coveralls commented Sep 28, 2016 •

edited

Loading

coveralls commented Sep 28, 2016 •

edited

Loading

coveralls commented Sep 28, 2016 •

edited

Loading

coveralls commented Sep 28, 2016 •

edited

Loading

tusharmakkar08 Sep 29, 2016 •

edited

Loading

coveralls commented Sep 29, 2016 •

edited

Loading

coveralls commented Sep 29, 2016 •

edited

Loading

coveralls commented Sep 29, 2016 •

edited

Loading

coveralls commented Sep 29, 2016 •

edited

Loading