Add ability to load non-resource files #235

hayfield · 2017-11-14T11:14:20Z

Fixes #234
Fixes #228

Allows custom Schema objects to be created (see: #234).

Also has a wider benefit in providing a consistent method for loading from files. For example, the load_as_dataset() utility function, and handling of encoding (see: #228).

By moving the functions to utilities, it becomes possible to load non-resource files

hayfield · 2017-11-14T11:16:52Z

iati/resources.py

@@ -9,7 +9,7 @@
 Example:
    To load a test XML file located in `my_test_file` and use it to create a `Dataset`::

-        dataset = iati.resources.load_as_dataset(iati.tests.resources.get_test_data_path('my_test_file'))
+        dataset = iati.utilities.load_as_dataset(iati.tests.resources.get_test_data_path('my_test_file'))


Nested function call

hayfield · 2017-11-14T11:18:21Z

iati/resources.py

@@ -9,7 +9,7 @@
 Example:


Module docstring about load_as_x not moved

hayfield · 2017-11-14T11:19:21Z

iati/tests/utilities.py

@@ -34,11 +34,11 @@
 SCHEMA_NAME_VALID = 'iati-activities-schema'
 """A string containing a valid Schema name."""

-XML_TREE_VALID = etree.fromstring(iati.tests.resources.load_as_string('valid_not_iati'))
+XML_TREE_VALID = etree.fromstring(iati.utilities.load_as_string(iati.tests.resources.get_test_data_path('valid_not_iati')))


load_as_tree

hayfield · 2017-11-14T11:19:26Z

iati/tests/utilities.py

 """An etree that is valid XML but not IATI XML."""
-XML_TREE_VALID_IATI = etree.fromstring(iati.tests.resources.load_as_string('valid_iati'))
+XML_TREE_VALID_IATI = etree.fromstring(iati.utilities.load_as_string(iati.tests.resources.get_test_data_path('valid_iati')))


load_as_tree

hayfield · 2017-11-14T11:19:31Z

iati/tests/utilities.py

 """A valid IATI etree."""
-XML_TREE_VALID_IATI_INVALID_CODE = etree.fromstring(iati.tests.resources.load_as_string('valid_iati_invalid_code'))
+XML_TREE_VALID_IATI_INVALID_CODE = etree.fromstring(iati.utilities.load_as_string(iati.tests.resources.get_test_data_path('valid_iati_invalid_code')))


load_as_tree

hayfield · 2017-11-14T11:19:48Z

iati/validator.py

@@ -699,7 +699,7 @@ def get_error_codes():
        Raise an error when there is a problem with non-base_exception-related errors.

    """
-    err_codes_str = iati.resources.load_as_string(iati.resources.get_lib_data_path('validation_err_codes.yaml'))
+    err_codes_str = iati.utilities.load_as_string(iati.resources.get_lib_data_path('validation_err_codes.yaml'))


Nested function call

Datasets can now directly handle the result of load_as_tree - this seems like a reasonable thing to expect

There is no clean way to undertake parameterisation of fixtures with fixtures. A stackoverflow answer is used as a base: https://stackoverflow.com/a/29407931 Fixtures need parametrizing like this due to use of the tmpdir fixture.

There are now cleaner ways to do things. This change updates the README to reflect this.

allthatilk · 2017-11-14T14:53:33Z

iati/data.py

@@ -78,7 +78,7 @@ def xml_str(self):

    @xml_str.setter
    def xml_str(self, value):


Can't tell if you should be ducking because there's no docstring. I assume not though.

Because it's a property, the getter is documented but the setter isn't.

allthatilk · 2017-11-14T14:59:31Z

iati/tests/resources.py

@@ -101,11 +103,11 @@ def get_test_ruleset_path(name, version=None):
    return os.path.join(PATH_TEST_DATA, iati.resources.get_folder_name_for_version(version), 'rulesets/{0}'.format(name) + iati.resources.FILE_RULESET_EXTENSION)


-def load_as_dataset(file_path):
+def load_as_dataset(relative_path):


Should this and load_as_str be 2 functions? They're very similar. Is it possible to do a try catch where the fail attempts to local the file in the second way?

Yes - they are different.

(one returns a string, the other a Dataset)

Yeah, I think I was half-asleep when I wrote this one... Not saying your code is dull or anything... 👀

I am such a professional ⭐️

allthatilk · 2017-11-14T15:10:45Z

iati/utilities.py

+    Todo:
+        Ensure all reasonably possible `OSError`s are documented here and in functions that call this.
+
+        Add error handling for when the specified file does not exist.


Important to be done before going to production. Would suggest doing it now tbh.

TODO should be removed since the error is a documented part of the API.

allthatilk · 2017-11-14T15:11:02Z

iati/utilities.py

+    Todo:
+        Ensure all reasonably possible OSErrors are documented here and in functions that call this.
+
+        Add error handling for when the specified file does not exist.


TODO should be removed since the error is a documented part of the API.

allthatilk · 2017-11-14T15:15:18Z

iati/utilities.py

+        Does not fully hide the lxml internal workings. This includes making reference to a private lxml type.
+
+    Todo:
+        Handle when the specified file can be accessed without issue, but it does not contain valid XML.


Again on the probs should do sooner rather than later front.

Pending adding version-independent tests.

allthatilk · 2017-11-14T15:16:10Z

iati/utilities.py

+        doc = etree.parse(path)
+        return doc
+    except OSError:
+        raise


Maybe more error info?

see TODO on the function 05bc5b7

dun

allthatilk

Looks good to me... probably... I'm very tired.

Move load_as_x functions to utilities

737744f

By moving the functions to utilities, it becomes possible to load non-resource files

hayfield added incomplete A PR that is in a state that is not ready for review. resources Relating to handling of resource (not code) files. labels Nov 14, 2017

hayfield commented Nov 14, 2017

View reviewed changes

hayfield added 4 commits November 14, 2017 11:23

Shuffle module docstrings to correlate with where code now is

3427db2

Un-nest function calls and clarify parameters

b031115

Use load_as_tree() to load data as trees

0bc79f0

Datasets can now directly handle the result of load_as_tree - this seems like a reasonable thing to expect

Fix lintin for #235

021d2d4

hayfield mentioned this pull request Nov 14, 2017

Add testrunner using pyiati and pytest IATI/IATI-Schemas#367

Merged

hayfield added 3 commits November 14, 2017 13:31

Test loading of non-resource files

e48e000

There is no clean way to undertake parameterisation of fixtures with fixtures. A stackoverflow answer is used as a base: https://stackoverflow.com/a/29407931 Fixtures need parametrizing like this due to use of the tmpdir fixture.

Fix #235 linting

a9e40c1

Update README with load_as_x utility functions

3a07478

There are now cleaner ways to do things. This change updates the README to reflect this.

hayfield requested a review from a team November 14, 2017 14:16

Remove out-of-date TODOs

04e7b0a

allthatilk previously requested changes Nov 14, 2017

View reviewed changes

Remove or update out-of-date TODOs

05bc5b7

hayfield requested review from dalepotter and allthatilk November 14, 2017 15:27

allthatilk approved these changes Nov 14, 2017

View reviewed changes

hayfield merged commit 42588bd into dev Nov 14, 2017

hayfield deleted the custom-schema-loading branch November 14, 2017 15:42

		@@ -78,7 +78,7 @@ def xml_str(self):

		@xml_str.setter
		def xml_str(self, value):

Add ability to load non-resource files #235

Add ability to load non-resource files #235

Conversation

hayfield commented Nov 14, 2017 • edited Loading

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

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

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

allthatilk left a comment

Choose a reason for hiding this comment

hayfield commented Nov 14, 2017 •

edited

Loading