#28 Use better Exception classes

[ewenchou]

For issue #28
Note: Some of the files and Exception usage you put in the issue aren't there anymore.

[rougeth]

Thanks Ewen, but I wanted something different. In those cases, creating a specific exception, like Django does, ImproperlyConfigured.

[ewenchou]

You mean like a custom Exception class? It would just be a subclass of Exception.
e.g.

class BotteryImproperlyConfigured(Exception):
    pass

Any specific additional requirements?

[rougeth]

Yep, just like that. Don't need Bottery in the name, and a docstring would be nice, like Bottery is somehow improperly configured.

Probably a file exceptions.py would be nice too.

[ewenchou]

Thanks, please check if updated PR is OK.

[rougeth]

Thank you very much @ewenchou \o/, do you mind rebasing your branch so that only your changes get merged?

[rougeth]

@ewenchou you did a merge, should be a rebase. This way, your PR is changing more things then it should. To fix this, probably cherry-picking is the best approach. Do you need help?

[ewenchou]

I thought I did... my steps were:

git add remote upstream https://github.com/rougeth/bottery.git
git fetch upstream
git rebase upstream/master
# I resolved one conflict in battery/cli.py where I accepted your changes

➜  bottery git:(4ea2215) ✗ git add bottery/cli.py
➜  bottery git:(4ea2215) ✗ git rebase --continue
Applying: Use bottery.exceptions.ImproperlyConfigured exception
Applying: Fix updated cli.py and missing ImproperlyConfigured class
Using index info to reconstruct a base tree...
M	bottery/cli.py
Falling back to patching base and 3-way merge...
➜  bottery git:(master) git status
On branch master
Your branch and 'origin/master' have diverged,
and have 11 and 3 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)
nothing to commit, working tree clean
➜  bottery git:(master) git pull
Merge made by the 'recursive' strategy.
➜  bottery git:(master) git log
➜  bottery git:(master) git status
On branch master
Your branch is ahead of 'origin/master' by 12 commits.
  (use "git push" to publish your local commits)
nothing to commit, working tree clean
➜  bottery git:(master) git log
➜  bottery git:(master) git diff 3883da4880e2d5f0478897f58189970197a6e7fb
➜  bottery git:(master) git push origin

Is that correct?

Maybe I should just do a new fork and put in my changes only 🤷‍♂️

[ewenchou]

I compared the files from ewenchou:master to rougeth:master and they are in-sync now. Only my changes for this issue should get merged in... but I'm not sure why in GitHub it is showing more changes.

You can manually check the diffs. I think it is safe to merge right now. Please confirm, thanks.

[rougeth]

The changes seems to be ok, what I'm concern is about the commits history. I would prefer to have a consistent tree. I'm sorry for all the trouble @ewenchou and thanks for your patience.

[ewenchou]

No problem. I'll re-clone and submit new PR.

[nicoddemus]

I suggest creating a base exception class and make every bottery exception subclass that. For example, BotteryException.

[rougeth]

@nicoddemus why is that?

[nicoddemus]

Oh missed the previous discussions.

Having a base exception is usually beneficial to clients, because they can just catch BotteryException if they want to catch any error from the bottery framework.

class BotteryException(Exception):
    """Base class Bottery exceptions"""

class ImproperlyConfigured(BotteryException):
    """Bottery is somehow improperly configured"""

And so on. This way you initially don't even need to actually specify a ton of exception subclasses, you can just raise BotteryException; if in the future you realize it is common for users to try to catch a specific error, you can create a subclass and existing clients will continue to work.

[rougeth]

Make total sense, thanks @nicoddemus, I'm going to open an issue for that.