error-handling: implement exercise #798

cmccandless · 2017-10-13T15:57:13Z

Resolves #742

TODO:

ilya-khadykin

Please read my comments

ilya-khadykin · 2017-10-21T09:42:38Z

config.json

+      "unlocked_by": null,
+      "difficulty": 3,
+      "topics": [
+        "exception handling"


We've switched to snake_case naming convention for topics to increase consistency among tracks, so could you please choose them from the following list - https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt

ilya-khadykin · 2017-10-21T09:43:11Z

exercises/error-handling/error_handling_test.py

+import unittest
+
+import error_handling as er
+


Could you please also leave a comment stating what version of canonical-data.json the tests were adopted as discussed in #784 if aplicable?
The format is:

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0

At this time, there is no canonical data for this exercise; these tests were adapted from the C# implemetation of this exercise. Is there a standard way of noting this?

If there is no tests in problem-specifications repo for this exercise, then it's Ok to leave it as is.

We agreed with @N-Parsons to use comment above to denote the version of canonical-data.json from which tests were adopted

N-Parsons · 2017-10-21T11:32:21Z

exercises/error-handling/README.md

+
+## Source
+
+Problem 6 at Project Euler [http://projecteuler.net/problem=6](http://projecteuler.net/problem=6)


This source isn't right - I presume that there's actually no source needed for this exercise?

Correct; I shall remove it. The included source section was in the README from another exercise from which I created this one.

@cmccandless if it was based on canonical-data, it will be good to open PR there to fix it

It was not. I copied the exercise-specific sections from the C# README and the track-specific sections (submitting, incomplete, etc) from another Python exercise. The source section was accidentally copied with the info from the other Python exercise.

N-Parsons · 2017-10-21T11:49:10Z

exercises/error-handling/example.py

+
+def filelike_objects_are_closed_on_exception(filelike_object):
+    with filelike_object:
+        filelike_object.close()


This isn't closing the filelike object on an exception, it's closing the filelike object and then raising an exception afterwards.

A better solution might be to have a was_open property and do_something method in the FileLike class, with the latter just raising some form of exception. Then it should be up to the user to call open, do_something, and close in that order. The tests can check that an exception was raised, that is_open is False, and that was_open is True.

It'd probably then be worth using the exercise's .meta/hints.md file to include something in the README about what methods exist in the filelike_object. The .meta/hints.md file is then included into the README when it's generated using the configlet - it's fine to just use copy-and-paste to put together the README, but also putting it in the hints file makes sure that it won't accidentally get lost in future.

Also, as a learner I would expect that a context manager could be used here (eg. with open(filelike_object) as f:) - it's the Pythonic way to handle files. I'm not sure what would need to be implemented in the FileLike class to make this work though. If we can't get a context manager working, then I think that the README needs to tell learners that context managers would be the typical way to do it with files, but that they won't work with the custom class here, because otherwise there is a massive amount of room for confusion and frustration.

The built-in open() method only takes a file name; you cannot implement a class to support open(filelike_object). However, I like the idea of do_something() raising an exception. See my latest changes.

N-Parsons · 2017-10-22T13:31:48Z

exercises/error-handling/error_handling_test.py

+        self.was_open = True
+
+    def __enter__(self):
+        self.open()



If you add return self here (plus make the change to __exit__), then this class will work with a context manager with an assignment.

N-Parsons · 2017-10-22T13:32:52Z

exercises/error-handling/error_handling_test.py

+        self.was_open = True
+
+    def __enter__(self):
+        self.open()

    def __exit__(self):


__exit__ needs to accept 4 arguments, but we don't need to use them for anything here, so we can just use def __exit__(self, *args):

N-Parsons · 2017-10-22T13:39:38Z

exercises/error-handling/error_handling_test.py

@@ -45,7 +53,12 @@ def test_filelike_objects_are_closed_on_exception(self):
        filelike_object.open()


I don't think that we should be passing an open object to the user's function. It seems strange to me that we'd pass something open to a function that (if we weren't just throwing an exception) may or may not close it - I would expect closing to be handled in the place that it's opened. I think it should be up to the user to open and close the filelike object however they see fit.

N-Parsons · 2017-10-22T14:05:11Z

exercises/error-handling/example.py

+            filelike_object.do_something()
+        except Exception:
+            filelike_object.close()
+            raise


You're actually mixing two error handling methods here - you have a context manager, but then you are explicitly closing an object using try ... except blocks to handle the exception behaviour.

Using try ... finally: (the finally block allows you to close the connection regardless of whether an exception is raised or not, and just doing except: raise is redundant)

filelike_object.open() try: filelike_object.do_something() finally: filelike_object.close()

Alternatively, a context manager can be used:

with filelike_object: filelike_object.do_something()

Or a context manager with an assignment:

with filelike_object as infile: infile.do_something()

I had forgotten about finally; thanks.

N-Parsons · 2017-10-23T18:40:57Z

exercises/error-handling/example.py

-            raise
+            fobj.do_something()
+        finally:
+            fobj.close()


As I stated previously, you don't need both a context manager and a try block for this - they are two different methods of error handling. Either you use a context manager, which will call the __enter__ and __exit__ methods that will open and close the object, OR you handle opening and closing explicitly, using a try ... finally block to ensure that the object is closed once the try block has finished.

This function should either be:

def filelike_objects_are_closed_on_exception(filelike_object): filelike_object.open() try: filelike_object.do_something() finally: filelike_object.close()

Or:

def filelike_objects_are_closed_on_exception(filelike_object): with filelike_object as fobj: fobj.do_something()

Or:

def filelike_objects_are_closed_on_exception(filelike_object): with filelike_object: filelike_object.do_something()

Apologies, I had somehow managed to convince myself that when an exception is raised from within the context manager object itself, __exit__() would not necessarily be called correctly, thus the need for try... finally inside with. Fix forthcoming...

N-Parsons

That's better, thanks!

I just noticed a couple of minor issues in the test file regarding convention - once these are resolved, I will merge the PR. Typically we use assertIs(actual, True) and assertIs(actual, False) rather that assertTrue/assertFalse, and assertEqual should follow the pattern assertEqual(actual, expected).

All of the conventions should be listed in the README for this repo, but some were missing and I've just submitted a PR to add them in.

…s/python into implement-error-handling

cmccandless · 2017-10-24T16:24:55Z

@N-Parsons Replaced assertTrue and assertFalse with assertIs.

N-Parsons

The tests need to follow the convention of assertEqual(actual, expected), currently you have assertEqual(expected, actual). The same is true for assertIs.

cmccandless · 2017-10-24T17:03:29Z

@N-Parsons Sorry, missed that one. I should be covered now.

There is currently no canonical data for this exercise.

N-Parsons · 2017-10-24T19:27:35Z

Thanks for your work on this, @cmccandless

cmccandless added 7 commits October 13, 2017 10:55

(WIP) error-handling: implement exercise

f9765ef

error-handling: update config.json

04f385b

error-handling: add README

b1c2967

error-handling: add solution template

135cdb7

error-handling: write test cases

e8e0ae1

write example solution

2a75c79

error-handling: fixes for flake8 compliance

2c9575a

cmccandless changed the title ~~(WIP) error-handling: implement exercise~~ error-handling: implement exercise Oct 15, 2017

ilya-khadykin added maintainer action required labels Oct 21, 2017

ilya-khadykin previously requested changes Oct 21, 2017

View reviewed changes

N-Parsons reviewed Oct 21, 2017

View reviewed changes

N-Parsons suggested changes Oct 21, 2017

View reviewed changes

cmccandless and others added 4 commits October 21, 2017 10:35

error-handling: change topics to snake_case

6543fcd

error-handling: remove incorrect Source section

6adccde

error-handling: improve use of context manager

68b8f22

Merge branch 'master' into implement-error-handling

fbaefd6

N-Parsons reviewed Oct 22, 2017

View reviewed changes

N-Parsons suggested changes Oct 22, 2017

View reviewed changes

cmccandless and others added 2 commits October 23, 2017 12:00

error-handling: further improve context manager implementation

24c7f79

Merge branch 'master' into implement-error-handling

995a836

N-Parsons suggested changes Oct 23, 2017

View reviewed changes

cmccandless and others added 2 commits October 23, 2017 14:00

error-handling: remove redundant error handling inside "with"

bc7eb88

Merge branch 'master' into implement-error-handling

6414219

N-Parsons suggested changes Oct 24, 2017

View reviewed changes

N-Parsons self-assigned this Oct 24, 2017

cmccandless added 2 commits October 24, 2017 11:16

error-handling: replace assertTrue and assertFalse with assertIs

d01e63e

Merge branch 'implement-error-handling' of github-personal:cmccandles…

1cb2a6d

…s/python into implement-error-handling

N-Parsons suggested changes Oct 24, 2017

View reviewed changes

error-handling: conform to parameter order convention

55b13a2

N-Parsons approved these changes Oct 24, 2017

View reviewed changes

Merge branch 'master' into implement-error-handling

b147fcd

N-Parsons merged commit 0d0d453 into exercism:master Oct 24, 2017

cmccandless deleted the implement-error-handling branch October 24, 2017 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error-handling: implement exercise #798

error-handling: implement exercise #798

cmccandless commented Oct 13, 2017 •

edited

Loading

ilya-khadykin left a comment

ilya-khadykin Oct 21, 2017

ilya-khadykin Oct 21, 2017

cmccandless Oct 21, 2017

ilya-khadykin Oct 21, 2017

N-Parsons Oct 21, 2017

cmccandless Oct 21, 2017

ilya-khadykin Oct 21, 2017

cmccandless Oct 21, 2017

N-Parsons Oct 21, 2017 •

edited

Loading

N-Parsons Oct 21, 2017

cmccandless Oct 21, 2017

N-Parsons Oct 22, 2017 •

edited

Loading

N-Parsons Oct 22, 2017 •

edited

Loading

N-Parsons Oct 22, 2017

N-Parsons Oct 22, 2017

cmccandless Oct 23, 2017

N-Parsons Oct 23, 2017

cmccandless Oct 23, 2017

N-Parsons left a comment

cmccandless commented Oct 24, 2017

N-Parsons left a comment

cmccandless commented Oct 24, 2017

N-Parsons commented Oct 24, 2017


		## Source

		Problem 6 at Project Euler [http://projecteuler.net/problem=6](http://projecteuler.net/problem=6)

		@@ -45,7 +53,12 @@ def test_filelike_objects_are_closed_on_exception(self):
		filelike_object.open()

		import unittest

		import error_handling as er

error-handling: implement exercise #798

error-handling: implement exercise #798

Conversation

cmccandless commented Oct 13, 2017 • edited Loading

ilya-khadykin 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

N-Parsons Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

N-Parsons Oct 22, 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

N-Parsons left a comment

Choose a reason for hiding this comment

cmccandless commented Oct 24, 2017

N-Parsons left a comment

Choose a reason for hiding this comment

cmccandless commented Oct 24, 2017

N-Parsons commented Oct 24, 2017

cmccandless commented Oct 13, 2017 •

edited

Loading

N-Parsons Oct 21, 2017 •

edited

Loading

N-Parsons Oct 22, 2017 •

edited

Loading

N-Parsons Oct 22, 2017 •

edited

Loading