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

Added different exception types #85

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Added different exception types #85

merged 2 commits into from
Jan 23, 2025

Conversation

irinascurtu
Copy link
Contributor

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@udidahan
Copy link
Member

Could we move the exception classes to the respective Sales, Billing, Shipping folders - getting rid of the Helper folder and namespace?

@dvdstelt
Copy link
Member

@udidahan I heard you asked for unique exceptions that allow us to group them in ServicePulse. But the current exceptions are business exceptions like BillingProcessingException. Normally we advocate against using exceptions like that and instead write code to deal with those alternate paths in the business process.

Would it not be better to throw DivideByZeroException or SqlException, even though we don't use any database? Just mimic technical exceptions that would normally occur.

@irinascurtu
Copy link
Contributor Author

Could we move the exception classes to the respective Sales, Billing, Shipping folders - getting rid of the Helper folder and namespace?
There are many things we could do, but I think we should think about priorities right now. I pushed a new version with those classes moved

Copy link
Member

@udidahan udidahan left a comment

Choose a reason for hiding this comment

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

This is better 👍🏼

@udidahan
Copy link
Member

@dvdstelt

Would it not be better to throw DivideByZeroException or SqlException, even though we don't use any database? Just mimic technical exceptions that would normally occur.

That would also be fine.

@irinascurtu irinascurtu merged commit 80892d9 into main Jan 23, 2025
2 checks passed
@irinascurtu irinascurtu deleted the exceptions branch January 23, 2025 14:10
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.

None yet

3 participants