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

refactor: move checkpoint and errors into separate module #1430

Merged
merged 6 commits into from
Jun 3, 2023

Conversation

roeap
Copy link
Collaborator

@roeap roeap commented Jun 3, 2023

Description

While doing #1409 it became evident, that our errors are somewhat organically grown and could do with some pruning. At the same time we are hoping to reorganize (#1136) delta-rs a bit, to make it easier to reason about.

This PR is a first attempt to introduce a more explicit approach to how we model our errors. Here I'd like to propose we work towards the approach taken in the object store crate - specifically have very specific errors in submodules, but do not surface those errors to the user. Rather collapse them into eventually a single error type.

Since delta-rs does much more things then object store, I believe we will eventually end up with some more top level errors, or have maybe a two level hierarch.

As a first step in this PR:

  • move checkpoints.rs into actions module (as proposed in Reorganize crate into fewer top-level modules #1136)
  • move top level errors into new errors module - at lest the ones defined in delta.rs
  • try and consolidate ActionError and CheckpointError which are now part of the action module.

As I needed to touch a bunch of imports anyhow, I took the liberty to organize them according to what to the best of my knowledge is the leading contender for what rust-fmt might do.

@wjones127 @rtyler @Blajda - opening this as a draft to get some feedback, as this is a somewhat intrusive change to the overall crate, in case you want to go another way.

Related Issue(s)

part of: #1136

Documentation

@github-actions github-actions bot added binding/python Issues for the Python package binding/rust Issues for the Rust crate rust labels Jun 3, 2023
@roeap
Copy link
Collaborator Author

roeap commented Jun 3, 2023

The refactoring went reasonably well, at the very least CheckpointError is contained in its module. I went ahead and renamed ActionError to ProtocolError, since I thought its reasonable that we should end up with an error type as such.

If folks agree I'd like to get this merged to do some follow ups, that should be much easier to reason about.

@roeap roeap marked this pull request as ready for review June 3, 2023 23:05
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

This all looks reasonable.

Is there a script yet we can run to order the use statements? Or are we doing that manually for now?

@roeap
Copy link
Collaborator Author

roeap commented Jun 3, 2023

I think i tried it once, but it required nightly rust. In principle though its part of rustfmt.

@roeap roeap merged commit 98af4dc into delta-io:main Jun 3, 2023
@roeap roeap deleted the checkpoint branch June 3, 2023 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/python Issues for the Python package binding/rust Issues for the Rust crate rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants