Skip to content

Conversation

@guzman-raphael
Copy link
Contributor

No description provided.

@guzman-raphael guzman-raphael marked this pull request as ready for review October 29, 2024 02:43
Copy link
Contributor

@Synicix Synicix left a comment

Choose a reason for hiding this comment

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

Looks good

@Synicix Synicix merged commit aec49ec into nauticalab:dev Oct 29, 2024
1 check passed
Copy link
Contributor

@eywalker eywalker left a comment

Choose a reason for hiding this comment

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

I believe there might be a problem with the handling of the Result<T> alias?

};
/// Shorthand for a Result that returns an `OrcaError`.
pub type Result<T> = result::Result<T, OrcaError>;
pub(crate) type Result<T> = result::Result<T, OrcaError>;
Copy link
Contributor

@eywalker eywalker Oct 29, 2024

Choose a reason for hiding this comment

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

Given we will have methods that will actually return this Result, wouldn't the unavailability of this type definition make the code uncompilable the moment those methods using Result return types are used outside of our crate? Does it act differently if this is a type alias?

use std::{collections::BTreeMap, fs, ops::Deref, path::PathBuf, result};
use tempfile::tempdir;

pub type Result<T> = result::Result<T, OrcaError>;
Copy link
Contributor

@eywalker eywalker Oct 29, 2024

Choose a reason for hiding this comment

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

Given users might also want to use our alias for Result if they work a lot with OrcaError, we might as well treat it as publicly facing interface?

@eywalker
Copy link
Contributor

After further digging, I learned that it is not necessary for type def to be shared publicly even if used as a return type for a public method as it is just a convenience alias available within the crate (if you so choose). So the concern over the code not compiling was not well founded - sorry! That being said, should we still consider making the alias publicly available? I see it's fairly common to make it public.

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.

3 participants