Skip to content

[test] check the error code when raising System Exit #128

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

Closed
Lee-W opened this issue Jan 29, 2020 · 9 comments
Closed

[test] check the error code when raising System Exit #128

Lee-W opened this issue Jan 29, 2020 · 9 comments
Assignees
Milestone

Comments

@Lee-W
Copy link
Member

Lee-W commented Jan 29, 2020

Goal
make unit tests more accurate

Description
In tests/commands/test_bump_command.py#L86, we do not check the error code. It's possible to raise the same exception but not our expected case. Maybe we should refactor them as custom exceptions?

@woile
Copy link
Member

woile commented Jan 29, 2020

Yes, custom exceptions makes sense. But this would be a breaking change, we can schedule it for 2.0

@Lee-W
Copy link
Member Author

Lee-W commented Jan 29, 2020

Maybe we can fix it in two steps. The first step is to check only the error code and merge it into 1.*. The second step is to refactor it into custom exceptions and release it in 2.0.

@woile
Copy link
Member

woile commented Jan 29, 2020

To be honest, I don't see much value in adding the checks for 1.x. The error code thing is an idea I had to give people flexibility to decide what to do on each error when writing configuration scripts, but I've never seen anyone do conditions on errors in bash, even less with commitizen.

So I say we don't waste time on it (be my guest if you want to do it, but I don't think it really adds value). And refactor into more expressive custom exceptions for 2.0

@woile woile added this to the 2.0 milestone Jan 29, 2020
@Lee-W
Copy link
Member Author

Lee-W commented Jan 31, 2020

Yes, that makes sense. Do we have any plan on when or how we release 2.0? Maybe a branch 2.0 so we can merge 2.0-only commits into the branch?

@woile
Copy link
Member

woile commented Jan 31, 2020

I've created a branch next, we can start sending PR there

@Lee-W
Copy link
Member Author

Lee-W commented May 23, 2020

I think the error code is quite useful when I use some of the new features recently. I'm thinking of refactoring these errors into classes but still have the same error code which makes it a non-breaking change. @woile What do you think?

@woile
Copy link
Member

woile commented May 23, 2020

Great idea! Go for it!

@woile
Copy link
Member

woile commented Jun 26, 2020

@Lee-W can we close this ticket?

@Lee-W
Copy link
Member Author

Lee-W commented Jun 26, 2020

Ah yes, I forgot it haha. Let's close it!

@Lee-W Lee-W closed this as completed Jun 26, 2020
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

No branches or pull requests

2 participants