Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Error-Handling]: instructions are unclear, and wrong programs pass the tests #1780

Open
Stigjb opened this issue May 14, 2019 · 11 comments
Open
Assignees
Labels
bug 🐛 smolder🍲 Lower-priority and/or future idea

Comments

@Stigjb
Copy link
Contributor

Stigjb commented May 14, 2019

I just did the error-handling exercise, and had several issues:

  • It was unclear at first what to do with inputs. Looking at the tests, it seemed that trying to convert the input to int was the right thing to try, but this is not mentioned in the readme or the stubs.
  • The test for whether a method throws an exception will pass any error that might occur, not just if it's intentionally raised:
def handle_error_by_throwing_exception():
    raise "(╯°□°)╯︵ ┻━┻"  # Raising a string is a TypeError, but passes the test
  • When returning a tuple, which element should be the result and which one the status? What should the result element be when it fails? No suggestion
  • No mention of advantages or disadvantages of the different kinds of error handling, or what is idiomatic Python.
@AnAccountForReportingBugs
Copy link
Contributor

The problem with the test example is that the assertRaises methods delete the traceback of the exception before they return them. The closest this could probably get would be:

with self.assertRaisesWithMessage(Exception) as err:
    er.handle_error_by_throwing_exception()
err_type = err.exception.__class__.__name__
code = inspect.getsourcelines(er.handle_error_by_throwing_exception)[0]
self.assertIn(err_type, "".join(code))

However, that is limited to checking whether the exception class name is referenced somewhere in the entire function, and not just the statement that actually raised it.

@cmccandless
Copy link
Contributor

The entire error-handling exercise is a bit problematic in its current implementation. It will likely be replaced with a proper Concept Exercise in v3.

@yawpitch
Copy link
Contributor

yawpitch commented Mar 8, 2020

Yeah, this exercise needs a complete rethink.

My best thought is that the dummy file include something like this:

# do not change the UnhappyError, but be prepared to handle it below.
class UnhappyError(Exception):
    pass

def convert_unhappy_to_none(func, *args, **kwargs):
    # you may move the following line, but it _must_ appear in your function
    return func(*args, **kwargs)

def convert_unhappy_to_bool(func, *args, **kwargs):
    # you may move the following line, but it _must_ appear in your function
    return func(*args, **kwargs)

def convert_unhappy_to_error_code(func, *args, **kwargs):
    # you may move the following line, but it _must_ appear in your function
    return func(*args, **kwargs)

def convert_unhappy_to_typeerror(func, *args, **kwargs):
    # you may move the following line, but it _must_ appear in your function
    return func(*args, **kwargs)

The test file can then import the UnhappyError and generate mocks that will raise it as a side_effect in appropriate circumstances. Then we'd just need tests to validate that everything worked when no error was raised as well as when an UnhappyError was raised ... we can probably relax our usual habit of ignoring what's actually in an error message and simply write the exercise so the student MUST pass-through the message the tests put into the UnhappyError verbatim when converting to a new error type.

@AnAccountForReportingBugs
Copy link
Contributor

If you are going to go that far, don't forget to include having to use the three types of raise statements. Also, you can monkey patch whatever you want using @patch so the exception can be generated without having to pass in a specific function to be called.

Thinking about this, it could almost be step 2 of another exercise. For example, a tour of the built-in functions or types. In the example, step one is doing the conversions and calls with normal input (e.g. turn "123" into 123 or bool(None) == False), and this step two is the same but with bad calls or raised exceptions (int("football") or set(123) raises an exception) to teach people how to handle them before things get too complicated. I'm not married to the idea of the built-ins, but it shows the general concept of extending a previous exercise.

@morganpartee
Copy link
Contributor

morganpartee commented Jan 18, 2021

I noticed this yesterday! I understand that the v3 version of the track will have a new version of this exercise, but could we fix this in the interim with more descriptive hints?

As it stands the student is left without a real direction to move to pass the tests, without just trying stuff out. If the intent is to show how to use with and raise, could we give them hints as to what we want in each function?

Edit - Glad to submit a PR if that makes sense for a short term fix.

@BethanyG
Copy link
Member

BethanyG commented Jan 18, 2021

@morganpartee -- Thanks so much for volunteering to help clarify this exercise! Since we are close to launching a beta of V3 (within the next two months), and it looks like this exercise needs a complete re-work, we've decided to mark it as deprecated (which will remove it from the website for the Python track) until we can get both the V3 error-handling concept exercises completed and the re-write of this exercise completed.

We would love if you'd like to help with either the V3 concept exercises or a re-write of this practice exercise...we just don't want you to do a bunch of "short-term fix" work that won't get used. If you would like to take on either the V3 exercises or a re-write of this one, please reach out and let us know. Thanks again.

BethanyG added a commit that referenced this issue Jan 18, 2021
Per discussions on [issue 1780](#1780), the `error-handling` exercise probably needs a full re-write.  Until that can be completed, we're deprecating the exercise for the Python track.
@BethanyG BethanyG linked a pull request Jan 18, 2021 that will close this issue
@morganpartee
Copy link
Contributor

Thanks @BethanyG! I'll definitely join in on v3. I'm behind the curve on developments there, but I'm glad to help.

BethanyG added a commit that referenced this issue Jan 18, 2021
* Marking `error-handling` Side Exercise Deprecated 

Per discussions on [issue 1780](#1780), the `error-handling` exercise probably needs a full re-write.  Until that can be completed, we're deprecating the exercise for the Python track.

* Changed topics to null for deprecated exercise

* Changed unlock & difficulty params for error-handling

Exercise has be deprecated.

* remove trailing whitespace

Co-authored-by: Corey McCandless <[email protected]>
@cmccandless
Copy link
Contributor

@BethanyG do we want to close this issue (and mark as resolved by #2281), or leave it open as a reminder to address this exercise after requisite Concept Exercises are complete?

@BethanyG
Copy link
Member

@cmccandless -- I think this would be good to leave open as a reminder. Otherwise, it might get lost in the shuffle.

Ultimately, we want to log both the error-handling concept exercise issues and a re-write of this practice exercise as its own issue, but I don't have time atm to do so -- so I think the best bet is to leave this pending.

@BethanyG BethanyG changed the title error-handling: instructions are unclear, and wrong programs pass the tests [V2] error-handling: instructions are unclear, and wrong programs pass the tests Jan 29, 2021
@github-actions
Copy link
Contributor

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@BethanyG BethanyG added discussion 💬 maintainer action required❕ A maintainer needs to take action on this. labels Jun 16, 2021
@BethanyG BethanyG self-assigned this Jun 16, 2021
@BethanyG BethanyG added the pinned 📌 Do no mark stale or invalid. label Jun 16, 2021
@BethanyG BethanyG added please review 👀 and removed maintainer action required❕ A maintainer needs to take action on this. pinned 📌 Do no mark stale or invalid. labels Jun 16, 2021
@BethanyG BethanyG changed the title [V2] error-handling: instructions are unclear, and wrong programs pass the tests [Error-Handling]: instructions are unclear, and wrong programs pass the tests Nov 9, 2021
@BethanyG
Copy link
Member

See also #2275 for additional considerations for the error handling concept cluster and set of exercises.

@BethanyG BethanyG added the smolder🍲 Lower-priority and/or future idea label May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 smolder🍲 Lower-priority and/or future idea
Projects
None yet
Development

No branches or pull requests

6 participants