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

Rename custom exceptions form *Exception to *Error #179

Merged
merged 1 commit into from Nov 19, 2022

Conversation

Secrus
Copy link
Member

@Secrus Secrus commented Jul 16, 2022

Resolves: #162

@dimbleby
Copy link
Contributor

nb this is a breaking change.

If you're going to make breaking changes, it's worth considering more cleanup than this.

  • Some but not all of these begin Cleo, consistency would be desirable
  • especially RuntimeError, which collides with the builtin RuntimeError, that's asking for mistakes.
  • What do RuntimeError and CleoValueError mean that is different from the builtin RuntimeError and ValueError?
  • What's the difference between a RuntimeError and a LogicError?
  • What's a CleoSimpleError? in what sense are some of these simple and others not?
  • and so on

I realise that you were likely just looking to do a small tidy-up and not get into all this. But breaking changes should be rare: and if you're going to make breaking changes anyway, then that's the opportunity to make such improvements.

@Secrus
Copy link
Member Author

Secrus commented Jul 17, 2022

nb this is a breaking change.

If you're going to make breaking changes, it's worth considering more cleanup than this.

  • Some but not all of these begin Cleo, consistency would be desirable
  • especially RuntimeError, which collides with the builtin RuntimeError, that's asking for mistakes.
  • What do RuntimeError and CleoValueError mean that is different from the builtin RuntimeError and ValueError?
  • What's the difference between a RuntimeError and a LogicError?
  • What's a CleoSimpleError? in what sense are some of these simple and others not?
  • and so on

I realise that you were likely just looking to do a small tidy-up and not get into all this. But breaking changes should be rare: and if you're going to make breaking changes anyway, then that's the opportunity to make such improvements.

Thanks for your input, I appreciate it. I will look into what you pointed out. I am aware that those are breaking changes, but as far as I know, the next release is gonna be 1.0.0 which could be a good point to introduce such changes, especially since we are working towards better code quality with more flake8 checks and the introduction of type checking via mypy.

@Secrus Secrus added this to the 1.0 milestone Sep 12, 2022
@Secrus Secrus force-pushed the rename-exceptions branch 4 times, most recently from 7bac59a to 0109854 Compare November 13, 2022 01:19
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; but I would like to see the doc strings introduced have consistent punctuation.

I'd also squash the two commits -- the entire codebase is broken after the first one, so I do not see any value in splitting them.

@neersighted neersighted merged commit 675ec7e into python-poetry:main Nov 19, 2022
@Secrus Secrus deleted the rename-exceptions branch November 20, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleo exceptions don't use pep8 naming conventions (and pep8-naming flake8 plugin doesn't seem to care)
3 participants