-
Notifications
You must be signed in to change notification settings - Fork 242
chore(dal): Single si_db::Error type, use si_db::* types from dal #6114
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
Conversation
8000
Dependency Review✅ No vulnerabilities or OpenSSF Scorecard issues found.Scanned FilesNone |
aca8fc6
to
bced0d6
Compare
Err(err) => return Err(ChangeSetError::User(err)), | ||
} | ||
} | ||
HistoryActor::User(user_pk) => User::get_by_pk(ctx, *user_pk).await?.pk(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
// TODO TransactionsError really needs to be moved to a common place accessible here and in dal | ||
#[remain::sorted] | ||
#[derive(Debug, Error, strum::EnumDiscriminants)] | ||
pub enum BaseTransactionsError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
#[remain::sorted] | ||
#[derive(thiserror::Error, Debug)] | ||
pub enum Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay one error!
// TODO TransactionsError really needs to be moved to a common place accessible here and in dal | ||
#[remain::sorted] | ||
#[derive(Debug, Error, strum::EnumDiscriminants)] | ||
pub enum SiDbTransactionsError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep this alive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because one thing at a time, honestly :) I think the right thing to do in the end is to call this TransactionsError
and remove dal::TransactionsError, because most of the things returning that error aren't actually transactions ... but that will be better done by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great to me!
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work!! One error type is the way to go too.
This is a sort of "cleanup PR" after the work to move DB types from dal into the si-db crate:
use si_db::User
insteadTesting