Add basic support for YAML data files #42
Conversation
Add support for YAML files, when PyYAML is installed. If PyYAML is not installed, any YAML file_data functions error out. Only files ending with .yml or .yaml are considered to be YAML files. All other files are loaded as JSON files.
| return | ||
|
|
||
| # Load the data as YAML or JSON | ||
| if data_file_path.endswith((".yml", ".yaml")): |
There was a problem hiding this comment.
Are there any other extensions you wish to support?
|
Off-topic: Shouldn't flake8 be run, on a separate worker instead of for all Python versions? |
Current coverage is 100%@@ master #42 diff @@
===================================
Files 1 1
Lines 88 104 +16
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 88 104 +16
Misses 0 0
Partials 0 0
|
|
I'm not sure how to add coverage for the no-YAML installed branches, unless a change is made to the API which I don't want to do since the current API is sufficient.
|
|
Oh, I forgot to mention, Fixes #30. |
|
whoops! I will now wait for someone else to say something. |
|
Looks good, and thanks for spotting the open file issue. I realise that code coverage has now decreased, because there isn't any test for the case where the user doesn't have yaml support installed. Do you think you could add one? |
| except Exception: | ||
| do_not_have_yaml_support = True | ||
| else: | ||
| do_not_have_yaml_support = False |
There was a problem hiding this comment.
As a matter of preference, I'd prefer to word things positively and avoid double negative statements like this one.
has_yaml_support makes things more readable. Below you can use not has_yaml_support for skipping the unit test...
|
The documentation should also be updated (even if just mentioning in https://github.com/txels/ddt/blob/master/docs/example.rst that file can be JSON or YAML). |
Change behaviour to close the data file once work is done. Also, pass the data file object is directly to the loaders, instead of reading and buffering the entire file and passing the buffer.
It would disallow for arbitary objects from being created and makes the file-loading safe.
It's not trivial. The thing is running that test would require the lack of PyYAML. That would increase the size of the build-matrix, and still, the reported coverage won't increase (unless codecov allows for some sort of these-are-related-builds flags). I could have a test-run without PyYAML but I'm not sure how that is to be done within the current method of dependency installation. I think I'm going to make another PR re-factoring the tests to be more comprehensive and easier to manage, after this PR... |
|
In honesty you could test the pyyaml absence by just patching |
|
But you'll still be missing the except clause around the import of yaml. On Sun, May 29, 2016, 8:22 PM Carles Barrobés notifications@github.com
|
|
True, but I can live with that. |
Specifically, for YAML files are missing and missing PyYAML support.
|
Okay. That's all the tests done and all the CI/Code-Quality checks satisfied. Are there any other outstanding issues? |
| @@ -270,3 +299,24 @@ def test_feed_data_with_invalid_identifier(): | |||
| 'test_data_with_invalid_identifier_1_32v2_g__Gmw845h_W_b53wi_' | |||
| ) | |||
| assert_equal(method(), '32v2 g #Gmw845h$W b53wi.') | |||
|
|
|||
|
|
|||
| @mock.patch('ddt._have_yaml', False) | |||
Documentation only change, skip ci
|
Merge on green. |
|
Would the release be a minor or major version bump? I would like major. But it's upto you. |
|
Bump. A gentle reminder close #30 and release this version. |
|
:) thanks for the reminder. Done, pushed and closed. Unfortunately pypi is having a fit at the moment (generating 503 errors) so it will take a bit longer for me to release the new version. Thanks again. |
Add support for YAML files, when PyYAML is installed. If PyYAML is not
installed, any YAML file_data functions error out.
Only files ending with .yml or .yaml are considered to be YAML files.
All other files are loaded as JSON files.