Skip to content

Conversation

@jonathanc-n
Copy link
Contributor

Copy link
Collaborator

@gabotechs gabotechs left a comment

Choose a reason for hiding this comment

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

Thanks! just a small comment and we can move this into main

@@ -1,4 +1,5 @@
mod distributed_codec;
pub mod errors;
Copy link
Collaborator

@gabotechs gabotechs Sep 25, 2025

Choose a reason for hiding this comment

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

Note how in all the modules of the project, we try to be strict about what's visible to the outside by not making the inner modules public. It would be fine to be consistent here and:

  • instead of blindly exposing the inner errors module:
Suggested change
pub mod errors;
mod errors;
  • Just expose the few things that are strictly needed:
pub(crate) use errors::{
    datafusion_error_to_tonic_status, map_flight_to_datafusion_error,
    map_status_to_datafusion_error,
};

Copy link
Contributor Author

@jonathanc-n jonathanc-n Sep 25, 2025

Choose a reason for hiding this comment

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

Yes this makes sense. I like the way this is done compared to Datafusion, lets you see everything that is public at a glance 😄

Copy link
Collaborator

@gabotechs gabotechs left a comment

Choose a reason for hiding this comment

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

Nice! as soon as pipelines are green I'll merge this

@gabotechs gabotechs merged commit 63570d3 into datafusion-contrib:main Sep 25, 2025
4 checks passed
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.

2 participants