-
Notifications
You must be signed in to change notification settings - Fork 79
Add Test Cases for Python 3.7 #720
base: master
Are you sure you want to change the base?
Changes from all commits
55b4a07
7c46758
f453d80
2fabaa7
4eb5043
5f0bfde
43a6a3e
391ba87
c1ea339
7934bcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -30,6 +30,7 @@ | |||
import pandas_profiling | ||||
except ImportError: | ||||
pass | ||||
import six | ||||
import sys | ||||
import yaml | ||||
|
||||
|
@@ -328,6 +329,8 @@ def parse_config(config, env, as_dict=True): | |||
config = {} | ||||
elif stripped[0] == '{': | ||||
config = json.loads(config) | ||||
elif six.PY3: | ||||
config = yaml.load(config, Loader=yaml.FullLoader) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are these loader changes addressing? This seems unrelated to Python 3.7 compat to me at first glance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am sorry that I cannot remember the reason clearly.
I remember that I built the datalab image with py37 and got into some troubles with pyyaml before. As the pyyaml version show as below, Line 108 in 8c2df84
I guess that pyyaml will be installed as different version in py2 and py3. As you could see in the py35 coverage test, there were warnings concerning YAMLLoadWarning.
Maybe there will be warnings or errors in py37, too. |
||||
else: | ||||
config = yaml.load(config) | ||||
if as_dict: | ||||
|
@@ -373,6 +376,8 @@ def parse_config_for_selected_keys(content, keys): | |||
return {}, None | ||||
elif stripped[0] == '{': | ||||
config = json.loads(content) | ||||
elif six.PY3: | ||||
config = yaml.load(content, Loader=yaml.FullLoader) | ||||
else: | ||||
config = yaml.load(content) | ||||
|
||||
|
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.
Thanks for incorporating the naming style from #728. It is shorter and a more standard pattern.
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.
Thanks you very much for your support.