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

Improve error formatting code. #151

Merged
merged 1 commit into from
Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from all 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: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ project adheres to
* The sass `Value::Variable` and `Item::VariableDeclaration` variants
now holds a `Name` rather than just a `String` for the variable name.
Also, both now holds a `SourcePos`.
* `Error::error` now takes an `Into<String>` argument (PR #151).

### Improvements

Expand All @@ -44,8 +45,13 @@ project adheres to
* The css `var(...)` function is now parsed as a proper function, and not
as a special string (PR #147).
* The null value can be quoted as an empty string (PR #147).
* Make `Debug` formatting of `rsass::Error` look like the `Display` output,
but without the "Error: " prefix. This makes the error display correctly
if returned from a main function. This also removed the "Error: " prefix
from a lot of message strings (PR #151).
* In error message, don't show ellipses for consecutive lines (PR #147).
* Somtimes a trailing comma in argument lists is preserved (PR #147).
* Simplified `main` of the command-line by returning a `Result` (PR #151).
* Update sass-spec test suite to 2022-06-21.
* Some cleanups.

Expand Down
20 changes: 12 additions & 8 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use std::convert::From;
use std::{fmt, io};

/// Most functions in rsass that returns a Result uses this Error type.
#[derive(Debug)]
pub enum Error {
/// An IO error encoundered on a specific path
Input(String, io::Error),
Expand Down Expand Up @@ -40,32 +39,37 @@ impl std::error::Error for Error {}

impl Error {
/// A generic error message.
pub fn error<T: AsRef<str>>(msg: T) -> Self {
Error::S(format!("Error: {}", msg.as_ref()))
pub fn error<T: Into<String>>(msg: T) -> Self {
Error::S(msg.into())
}
}

impl fmt::Display for Error {
fn fmt(&self, out: &mut fmt::Formatter) -> fmt::Result {
write!(out, "Error: {:?}", self)
}
}

impl fmt::Debug for Error {
fn fmt(&self, out: &mut fmt::Formatter<'_>) -> fmt::Result {
match *self {
Error::S(ref s) => write!(out, "{}", s),
Error::Input(ref p, ref e) => {
write!(out, "Failed to read {:?}: {}", p, e)
}
Error::BadArgument(ref name, ref problem) => {
write!(out, "Error: ${}: {}", name, problem)
write!(out, "${}: {}", name, problem)
}
Error::ParseError(ref err) => err.fmt(out),
Error::ImportLoop(ref pos, ref oldpos) => {
writeln!(out, "Error: This file is already being loaded.")?;
writeln!(out, "This file is already being loaded.")?;
pos.show_detail(out, '^', " new load")?;
writeln!(out)?;
oldpos.show_detail(out, '=', " original load")?;
pos.show_files(out)
}
Error::BadCall(ref msg, ref callpos, ref declpos) => {
msg.fmt(out)?;
writeln!(out)?;
writeln!(out, "{}", msg)?;
if let Some(declpos) = declpos {
if callpos.same_file_as(declpos) {
show_in_file(
Expand All @@ -86,7 +90,7 @@ impl fmt::Display for Error {
}
}
Error::Invalid(ref what, ref pos) => {
writeln!(out, "Error: {}", what)?;
writeln!(out, "{}", what)?;
pos.show(out)
}
Error::BadRange(ref err) => err.fmt(out),
Expand Down
11 changes: 2 additions & 9 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,9 @@ use rsass::{
};
use std::io::{stdout, Write};
use std::path::PathBuf;
use std::process::exit;

fn main() {
match Args::parse().run() {
Ok(()) => (),
Err(err) => {
eprintln!("{}", err);
exit(1);
}
}
fn main() -> Result<(), Error> {
Args::parse().run()
}

#[derive(Parser)]
Expand Down
12 changes: 5 additions & 7 deletions src/output/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ fn handle_item(
{
if !with.is_empty() {
return Err(Error::BadCall(
"Error: Built-in modules can\'t be configured."
.into(),
"Built-in modules can\'t be configured.".into(),
pos.clone(),
None,
));
Expand Down Expand Up @@ -117,7 +116,7 @@ fn handle_item(
})?
} else {
return Err(Error::BadCall(
"Error: Can't find stylesheet to import.".into(),
"Can't find stylesheet to import.".into(),
pos.clone(),
None,
));
Expand All @@ -130,8 +129,7 @@ fn handle_item(
{
if !with.is_empty() {
return Err(Error::BadCall(
"Error: Built-in modules can\'t be configured."
.into(),
"Built-in modules can\'t be configured.".into(),
pos.clone(),
None,
));
Expand Down Expand Up @@ -221,7 +219,7 @@ fn handle_item(
|| x.is_css_url())
{
return Err(Error::BadCall(
"Error: Can't find stylesheet to import.".into(),
"Can't find stylesheet to import.".into(),
pos.clone(),
None,
));
Expand Down Expand Up @@ -360,7 +358,7 @@ fn handle_item(
})?;
} else {
return Err(Error::BadCall(
"Error: Undefined mixin.".into(),
"Undefined mixin.".into(),
pos.clone(),
None,
));
Expand Down
8 changes: 4 additions & 4 deletions src/sass/formal_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,25 +159,25 @@ impl fmt::Display for ArgsError {
match self {
ArgsError::TooManyPos(n, m) => write!(
out,
"Error: Only {} positional argument{} allowed, but {} {} passed.",
"Only {} positional argument{} allowed, but {} {} passed.",
n,
if *n != 1 { "s" } else { "" },
m,
if *m != 1 { "were" } else { "was" },
),
ArgsError::TooMany(n, m) => write!(
out,
"Error: Only {} argument{} allowed, but {} {} passed.",
"Only {} argument{} allowed, but {} {} passed.",
n,
if *n != 1 { "s" } else { "" },
m,
if *m != 1 { "were" } else { "was" },
),
ArgsError::Missing(name) => {
write!(out, "Error: Missing argument ${}.", name)
write!(out, "Missing argument ${}.", name)
}
ArgsError::Unexpected(name) => {
write!(out, "Error: No argument named ${}.", name)
write!(out, "No argument named ${}.", name)
}
ArgsError::Eval(e) => e.fmt(out),
}
Expand Down
2 changes: 1 addition & 1 deletion src/sass/functions/color/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ fn bad_arg(err: ArgsError, name: &Name, args: &FormalArgs) -> Error {

fn not_in_module(nm: &Name, col: &Value, an: &Name, av: &Value) -> Error {
Error::S(format!(
"Error: The function {0}() isn\'t in the sass:color module.\n\
"The function {0}() isn\'t in the sass:color module.\n\
\nRecommendation: color.adjust({1}, ${2}: {3})\n\
\nMore info: https://sass-lang.com/documentation/functions/color#{0}",
nm,
Expand Down
2 changes: 1 addition & 1 deletion src/sass/functions/meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub fn create_module() -> Scope {
} else if let Some(f) = get_function(s, module, name.value())? {
Ok(Value::Function(name.value().into(), Some(f)))
} else {
Err(Error::S(format!("Error: Function not found: {}", name)))
Err(Error::S(format!("Function not found: {}", name)))
}
}
);
Expand Down
10 changes: 5 additions & 5 deletions src/sass/mixin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl MixinDecl {
.map_err(|e| e.decl_called(call_pos2, pos))?;
let call_pos2 = call_pos.clone();
let url = get_string(&argscope, name!(url)).map_err(|e| {
Error::BadCall(e.to_string(), call_pos2, None)
Error::BadCall(format!("{:?}", e), call_pos2, None)
})?;
let call_pos2 = call_pos.clone();
let with = get_opt_map(&argscope, name!(with))
Expand All @@ -85,7 +85,7 @@ impl MixinDecl {
} else {
return Err(Error::BadCall(
format!(
"Error: Built-in module {} can't be configured.",
"Built-in module {} can't be configured.",
url.value()
),
call_pos.clone(),
Expand All @@ -98,7 +98,7 @@ impl MixinDecl {
.find_file_use(url.value(), call_pos.clone())?
.ok_or_else(|| {
Error::BadCall(
"Error: Can't find stylesheet to import.".into(),
"Can't find stylesheet to import.".into(),
call_pos2,
None,
)
Expand Down Expand Up @@ -160,10 +160,10 @@ pub(crate) fn get_opt_map(
.try_into()
.map_err(|e| match e {
ValueToMapError::Root(err) => {
format!("Error: ${}: {}", name, err)
format!("${}: {}", name, err)
}
ValueToMapError::Key(err) => {
format!("Error: ${} key: {}", name, err)
format!("${} key: {}", name, err)
}
})
.map(Some),
Expand Down
3 changes: 1 addition & 2 deletions src/sass/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,11 @@ impl Value {
Some(decl),
),
Error::Invalid(Invalid::AtError(msg), _) => {
let msg = format!("Error: {}", msg);
Error::BadCall(msg, pos.clone(), None)
}
e => {
let pos = pos.clone().opt_in_calc();
Error::BadCall(e.to_string(), pos, None)
Error::BadCall(format!("{:?}", e), pos, None)
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion tests/spec/main.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! Tests auto-converted from "sass-spec/spec"
//! version 50ee810b, 2022-06-21 17:35:49 -0700.
//! version dcff661e, 2022-07-06 13:41:49 -0700.
//! See <https://github.com/sass/sass-spec> for source material.\n
//! The following tests are excluded from conversion:
//! ["core_functions/selector/extend", "core_functions/selector/is_superselector", "core_functions/selector/unify", "directives/extend", "libsass-todo-issues/issue_221262.hrx", "libsass-todo-issues/issue_221292.hrx", "libsass/unicode-bom/utf-16-big", "libsass/unicode-bom/utf-16-little", "non_conformant/scss/huge.hrx", "non_conformant/scss/multiline-var.hrx"]
Expand Down