Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions dfx/src/commands/canister/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,13 @@ where

let canister_name = args.value_of("canister_name").unwrap();
let canister_info = CanisterInfo::load(config, canister_name)?;
// Read the config.
let canister_id = canister_info.get_canister_id().ok_or_else(|| {
DfxError::CannotFindBuildOutputForCanister(canister_info.get_name().to_owned())
})?;

let method_name = args.value_of("method_name").unwrap();
let method_name = args.value_of("method_name").ok_or_else(|| {
DfxError::InvalidArgument("method name argument provided invalid".to_string())
Comment thread
This conversation was marked as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"method_name"

Comment thread
eftychis marked this conversation as resolved.
Outdated
})?;
let arguments: Option<&str> = args.value_of("argument");
let arg_type: Option<&str> = args.value_of("type");

Expand Down
6 changes: 4 additions & 2 deletions dfx/src/commands/canister/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,13 @@ where

let canister_name = args.value_of("canister_name").unwrap();
let canister_info = CanisterInfo::load(config, canister_name)?;
// Read the config.
let canister_id = canister_info.get_canister_id().ok_or_else(|| {
DfxError::CannotFindBuildOutputForCanister(canister_info.get_name().to_owned())
})?;

let method_name = args.value_of("method_name").unwrap();
let method_name = args
.value_of("method_name")
.ok_or_else(|| DfxError::InvalidArgument("method name".to_string()))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should at least be consistent in the error messages. The InvalidArgument in call.rs should have the argument name as message. Also, this should use snake case as it's the name of the argument showing up in help.

let arguments: Option<&str> = args.value_of("argument");
let arg_type: Option<&str> = args.value_of("type");

Expand Down
10 changes: 8 additions & 2 deletions dfx/src/commands/canister/request_status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,14 @@ pub fn exec<T>(env: &T, args: &ArgMatches<'_>) -> DfxResult
where
T: ClientEnv,
{
let request_id = RequestId::from_str(&args.value_of("request_id").unwrap()[2..])
.map_err(|e| DfxError::InvalidArgument(format!("Invalid request ID: {:?}", e)))?; // FIXME Default formatter for RequestIdFromStringError
let request_id = RequestId::from_str(
&args.value_of("request_id").ok_or_else(|| {
DfxError::InvalidArgument(
"failed to retrieve request_id -- required argument".to_string(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

InvalidArgument should take the name of the argument as inner.

)
})?[2..],
)
.map_err(|e| DfxError::InvalidArgument(format!("invalid request ID: {:?}", e)))?; // FIXME Default formatter for RequestIdFromStringError

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels like it's a different error, not InvalidArgument. Maybe we need a validator here instead (Clap accepts validators which returns custom error messages).

let request_status = request_status(env.get_client(), request_id);
let mut runtime = Runtime::new().expect("Unable to create a runtime");
match runtime.block_on(request_status) {
Expand Down
4 changes: 3 additions & 1 deletion dfx/src/commands/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ pub fn exec<T: ProjectConfigEnv>(env: &T, args: &ArgMatches<'_>) -> DfxResult {
.ok_or(DfxError::CommandMustBeRunInAProject)?
.clone();

let config_path = args.value_of("config_path").unwrap();
let config_path = args
.value_of("config_path")
.ok_or_else(|| DfxError::InvalidArgument("config path".to_string()))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Snake case.


// We replace `.` with `/` so the user can use `path.value.field` instead of forcing him
// to use `path/value/field`. Since none of our keys have slashes or tildes in them it
Expand Down
20 changes: 15 additions & 5 deletions dfx/src/commands/new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ where
T: BinaryCacheEnv + PlatformEnv + VersionEnv,
{
let dry_run = args.is_present(DRY_RUN);
let project_name = Path::new(args.value_of(PROJECT_NAME).unwrap());
let project_name_path = args
.value_of(PROJECT_NAME)
.ok_or_else(|| DfxError::InvalidArgument("project path".to_string()))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Snake case.

let project_name = Path::new(project_name_path);

if project_name.exists() {
return Err(DfxError::ProjectExists);
Expand All @@ -127,6 +130,10 @@ where
}

let mut new_project_files = assets::new_project_files()?;
let project_name_str = project_name
.to_str()
.ok_or_else(|| DfxError::InvalidArgument("project name".to_string()))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Snake case.


for file in new_project_files.entries()? {
let mut file = file?;

Expand All @@ -135,18 +142,21 @@ where
}

let mut s = String::new();
file.read_to_string(&mut s).unwrap();
file.read_to_string(&mut s)
.or_else(|e| Err(DfxError::Io(e)))?;

// Perform replacements.
let s = s.replace("{project_name}", project_name.to_str().unwrap());
let s = s.replace("{project_name}", project_name_str);
let s = s.replace("{dfx_version}", dfx_version);

// Perform path replacements.
let p = PathBuf::from(
project_name
.join(file.header().path()?)
.to_str()
.unwrap()
.ok_or_else(|| {
Comment thread
hansl marked this conversation as resolved.
Outdated
Comment thread
eftychis marked this conversation as resolved.
Outdated
DfxError::InvalidArgument("project name path or file header".to_string())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should never happen. Maybe we need an Impossible(String) error variant for cases like these? This would only happen if a non-UTF8 character is in the path, which would break above for project_name and in the build script for file.header().path().

})?
Comment thread
eftychis marked this conversation as resolved.
Outdated
.replace("__dot__", ".")
.as_str(),
);
Expand Down Expand Up @@ -192,7 +202,7 @@ where
include_str!("../../assets/welcome.txt"),
dfx_version,
assets::dfinity_logo(),
project_name.to_str().unwrap()
project_name_str
);

Ok(())
Expand Down
20 changes: 16 additions & 4 deletions dfx/src/config/dfinity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,11 +218,23 @@ impl Config {
&self.config
}

pub fn get_project_root(&self) -> &Path {
// a configuration path contains a file name specifically. As
// such we should be returning at least root as parent. If
// this is invariance is broken, we must fail.
self.path.parent().expect(
"An incorrect configuration path was set with no parent, i.e. did not include root",
)
}

pub fn save(&self) -> DfxResult {
std::fs::write(
&self.path,
serde_json::to_string_pretty(&self.json).unwrap(),
)?;
let json_pretty = serde_json::to_string_pretty(&self.json).or_else(|e| {
Err(DfxError::InvalidData(format!(
"Failed to serialize -- {}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Failed to serialize dfx.json: {}" for consistency with other error messages.

e
)))
})?;
std::fs::write(&self.path, json_pretty)?;
Ok(())
}
}
Expand Down
27 changes: 18 additions & 9 deletions dfx/src/lib/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub use cache::CacheErrorKind;

// TODO: refactor this enum into a *Kind enum and a struct DfxError.
#[derive(Debug)]
/// Provides dfx user facing errors.
pub enum DfxError {
/// An error happened during build.
BuildError(BuildErrorKind),
Expand All @@ -30,24 +31,32 @@ pub enum DfxError {
/// An unknown command was used. The argument is the command itself.
UnknownCommand(String),

// Cannot create a new project because the directory already exists.
/// Cannot create a new project because the directory already exists.
ProjectExists,

// Not in a project.
/// Not in a project.
CommandMustBeRunInAProject,

// The client returned an error. It normally specifies the error as an
// HTTP status (so 400-599), and has a string as the error message.
// Once the client support errors from the public spec or as an enum,
// we should update this type.
// We don't use StatusCode here because the client might return some other
// number if they support public spec's errors (< 100).
/// The client returned an error. It normally specifies the error as an
/// HTTP status (so 400-599), and has a string as the error message.
/// Once the client support errors from the public spec or as an enum,
/// we should update this type.
/// We don't use StatusCode here because the client might return some other
/// number if they support public spec's errors (< 100).
ClientError(u16, String),

/// This option is used when the source/cause of the error is
/// ambiguous. If the cause is known use or add a new option.
Unknown(String),

// Configuration path does not exist in the config file.
/// Configuration path does not exist in the config file.
ConfigPathDoesNotExist(String),
/// Argument provided is invalid.
InvalidArgument(String),
#[allow(dead_code)]
/// Configuration provided is invalid.
InvalidConfiguration(String),
/// Data provided is invalid.
InvalidData(String),

// The ide server shouldn't be started from a terminal
Expand Down
5 changes: 4 additions & 1 deletion lib/serde_idl/src/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,11 +482,14 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
let value = visitor.visit_enum(Compound::new(&mut self, Style::Enum { len, fs }))?;
Ok(value)
}

/// Deserialize identifier.
/// # Panics
/// *Will Panic* when identifier name is None.
fn deserialize_identifier<V>(self, visitor: V) -> Result<V::Value>
where
V: Visitor<'de>,
{
// N.B. Here we want to panic as it indicates a logical error.
let label = self.field_name.as_ref().unwrap();
let v = match label {
FieldLabel::Named(name) => visitor.visit_str(name),
Expand Down