8000 chore(dal): Single si_db::Error type, use si_db::* types from dal by jkeiser · Pull Request #6114 · systeminit/si · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 1 commit into from
May 14, 2025

Conversation

jkeiser
Copy link
Contributor
@jkeiser jkeiser commented May 14, 2025

This is a sort of "cleanup PR" after the work to move DB types from dal into the si-db crate:

  • Stops re-exporting si_db types from dal, make callers use si_db::User instead
    • This helps us tell easily whether we really depend on the dal (should make it easier to tell when we can wean a caller or endpoint off the DAL)
    • Having done this, I'm no longer sure this needs to be done for everything and might not do it in the future; but this one's done
  • Moves all the si-db error types to a single si_db::Error type, to reduce future churn
    • except SiDbTransactionsError, which is still separate because of some weirdness around dal::TransactionsError that I haven't got around to thinking through yet

Testing

  • Integration tests pass

re-exporting

@jkeiser jkeiser requested a review from nickgerace May 14, 2025 15:12
Copy link
github-actions bot commented May 14, 2025
8000

Dependency Review

✅ No vulnerabilities or OpenSSF Scorecard issues found.

Scanned Files

None

@jkeiser jkeiser force-pushed the jkeiser/si-db-errors branch from aca8fc6 to bced0d6 Compare May 14, 2025 15:29
@jkeiser jkeiser changed the title chore(dal): Single si_db::Error type,use si_db::* types from dal chore(dal): Single si_db::Error type, use si_db::* types from dal May 14, 2025
Err(err) => return Err(ChangeSetError::User(err)),
}
}
HistoryActor::User(user_pk) => User::get_by_pk(ctx, *user_pk).await?.pk(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment on lines -31 to -34
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment on lines +40 to +42
#[remain::sorted]
#[derive(thiserror::Error, Debug)]
pub enum Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay one error!

Comment on lines +13 to +16
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why keep this alive?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great to me!

Copy link
Contributor
@nickgerace nickgerace left a 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.

@nickgerace nickgerace added this pull request to the merge queue May 14, 2025
Merged via the queue into main with commit a36ba9b May 14, 2025
10 checks passed
@nickgerace nickgerace deleted the jkeiser/si-db-errors branch May 14, 2025 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0