Skip to content

refactor: Use Origin for Snippet::origin #221

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

refactor: Use Origin for Snippet::origin #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
113 changes: 66 additions & 47 deletions src/renderer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,14 +239,14 @@ impl Renderer {
group.elements.iter().find_map(|s| match &s {
Element::Cause(cause) => {
if cause.markers.iter().any(|m| m.kind.is_primary()) {
Some(cause.origin)
Some(cause.origin.as_ref())
} else {
None
}
}
Element::Origin(origin) => {
if origin.primary {
Some(Some(origin.origin))
Some(Some(origin))
} else {
None
}
Expand All @@ -260,30 +260,30 @@ impl Renderer {
.iter()
.find_map(|group| {
group.elements.iter().find_map(|s| match &s {
Element::Cause(cause) => Some(cause.origin),
Element::Origin(origin) => Some(Some(origin.origin)),
Element::Cause(cause) => Some(cause.origin.as_ref()),
Element::Origin(origin) => Some(Some(origin)),
_ => None,
})
})
.unwrap_or_default(),
);
let group_len = message.groups.len();
for (g, group) in message.groups.into_iter().enumerate() {
for (g, group) in message.groups.iter().enumerate() {
let mut buffer = StyledBuffer::new();
let primary_origin = group
.elements
.iter()
.find_map(|s| match &s {
Element::Cause(cause) => {
if cause.markers.iter().any(|m| m.kind.is_primary()) {
Some(cause.origin)
Some(cause.origin.as_ref())
} else {
None
}
}
Element::Origin(origin) => {
if origin.primary {
Some(Some(origin.origin))
Some(Some(origin))
} else {
None
}
Expand All @@ -295,8 +295,8 @@ impl Renderer {
.elements
.iter()
.find_map(|s| match &s {
Element::Cause(cause) => Some(cause.origin),
Element::Origin(origin) => Some(Some(origin.origin)),
Element::Cause(cause) => Some(cause.origin.as_ref()),
Element::Origin(origin) => Some(Some(origin)),
_ => None,
})
.unwrap_or_default(),
Expand Down Expand Up @@ -490,26 +490,27 @@ impl Renderer {
labels = Some(labels_inner);
}

if let Some(origin) = cause.origin {
let mut origin = Origin::new(origin);
if let Some(mut origin) = cause.origin.clone() {
origin.primary = true;

let source_map = SourceMap::new(cause.source, cause.line_start);
let (_depth, annotated_lines) =
source_map.annotated_lines(cause.markers.clone(), cause.fold);
if origin.line.is_none() || origin.char_column.is_none() {
let source_map = SourceMap::new(cause.source, cause.line_start);
let (_depth, annotated_lines) =
source_map.annotated_lines(cause.markers.clone(), cause.fold);

if let Some(primary_line) = annotated_lines
.iter()
.find(|l| l.annotations.iter().any(LineAnnotation::is_primary))
.or(annotated_lines.iter().find(|l| !l.annotations.is_empty()))
{
origin.line = Some(primary_line.line_index);
if let Some(first_annotation) = primary_line
.annotations
if let Some(primary_line) = annotated_lines
.iter()
.min_by_key(|a| (Reverse(a.is_primary()), a.start.char))
.find(|l| l.annotations.iter().any(LineAnnotation::is_primary))
.or(annotated_lines.iter().find(|l| !l.annotations.is_empty()))
{
origin.char_column = Some(first_annotation.start.char + 1);
origin.line = Some(primary_line.line_index);
if let Some(first_annotation) = primary_line
.annotations
.iter()
.min_by_key(|a| (Reverse(a.is_primary()), a.start.char))
{
origin.char_column = Some(first_annotation.start.char + 1);
}
}
}

Expand Down Expand Up @@ -784,32 +785,40 @@ impl Renderer {
buffer: &mut StyledBuffer,
max_line_num_len: usize,
snippet: &Snippet<'_, Annotation<'_>>,
primary_origin: Option<&str>,
primary_origin: Option<&Origin<'_>>,
sm: &SourceMap<'_>,
annotated_lines: &[AnnotatedLineInfo<'_>],
multiline_depth: usize,
is_cont: bool,
) {
if let Some(origin) = snippet.origin {
let mut origin = Origin::new(origin);
if let Some(mut origin) = snippet.origin.clone() {
// print out the span location and spacer before we print the annotated source
// to do this, we need to know if this span will be primary
let is_primary = primary_origin == Some(origin.origin);
let is_primary = origin.primary || primary_origin == Some(&origin);

if is_primary {
origin.primary = true;
if let Some(primary_line) = annotated_lines
.iter()
.find(|l| l.annotations.iter().any(LineAnnotation::is_primary))
.or(annotated_lines.iter().find(|l| !l.annotations.is_empty()))
{
origin.line = Some(primary_line.line_index);
if let Some(first_annotation) = primary_line
.annotations

if origin.line.is_none() || origin.char_column.is_none() {
if let Some(primary_line) = annotated_lines
.iter()
.min_by_key(|a| (Reverse(a.is_primary()), a.start.char))
.find(|l| l.annotations.iter().any(LineAnnotation::is_primary))
.or(annotated_lines.iter().find(|l| !l.annotations.is_empty()))
{
origin.char_column = Some(first_annotation.start.char + 1);
if origin.line.is_none() {
origin.line = Some(primary_line.line_index);
}

// Always set the char column, as it is either `None`
// or we set the line and now need to set the column on
// that line.
if let Some(first_annotation) = primary_line
.annotations
.iter()
.min_by_key(|a| (Reverse(a.is_primary()), a.start.char))
{
origin.char_column = Some(first_annotation.start.char + 1);
}
}
}
} else {
Expand All @@ -829,10 +838,12 @@ impl Renderer {
buffer_msg_line_offset,
max_line_num_len + 1,
);
if let Some(first_line) = annotated_lines.first() {
origin.line = Some(first_line.line_index);
if let Some(first_annotation) = first_line.annotations.first() {
origin.char_column = Some(first_annotation.start.char + 1);
if origin.line.is_none() {
if let Some(first_line) = annotated_lines.first() {
origin.line = Some(first_line.line_index);
if let Some(first_annotation) = first_line.annotations.first() {
origin.char_column = Some(first_annotation.start.char + 1);
}
}
}
}
Expand Down Expand Up @@ -1648,7 +1659,7 @@ impl Renderer {
suggestion: &Snippet<'_, Patch<'_>>,
max_line_num_len: usize,
sm: &SourceMap<'_>,
primary_origin: Option<&str>,
primary_origin: Option<&Origin<'_>>,
is_cont: bool,
) {
let suggestions = sm.splice_lines(suggestion.markers.clone());
Expand All @@ -1671,14 +1682,22 @@ impl Renderer {
ElementStyle::LineNumber,
);
}
if suggestion.origin != primary_origin {
if let Some(origin) = suggestion.origin {
let (loc, _) = sm.span_to_locations(parts[0].span.clone());

if suggestion.origin.as_ref() != primary_origin && primary_origin.is_some() {
if let Some(origin) = suggestion.origin.clone() {
let (line, char_col) =
if let (Some(line), Some(char_col)) = (origin.line, origin.char_column) {
(line, char_col)
} else {
let (loc, _) = sm.span_to_locations(parts[0].span.clone());
(loc.line, loc.char + 1)
};

// --> file.rs:line:col
// |
let arrow = self.file_start();
buffer.puts(row_num - 1, 0, arrow, ElementStyle::LineNumber);
let message = format!("{}:{}:{}", origin, loc.line, loc.char + 1);
let message = format!("{}:{}:{}", origin.origin, line, char_col);
if is_cont {
buffer.append(row_num - 1, &message, ElementStyle::LineAndColumn);
} else {
Expand Down
48 changes: 43 additions & 5 deletions src/snippet.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use Snippet::origin if you are using a Snippet and Origin on its own if you need the file details.

maybe that should be put in some docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know what you think of the docs I added

Copy link
Contributor

Choose a reason for hiding this comment

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

But when would you use Snippet::origin(Origin')?

Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ pub struct Title<'a> {
/// A source view [`Element`] in a [`Group`]
#[derive(Clone, Debug)]
pub struct Snippet<'a, T> {
Comment on lines 164 to 166
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A source view [`Element`] in a [`Group`]
#[derive(Clone, Debug)]
pub struct Snippet<'a, T> {
/// A source view [`Element`] in a [`Group`]
///
/// If you do not have [source][Snippet::source] available, see instead [`Origin`]
#[derive(Clone, Debug)]
pub struct Snippet<'a, T> {

pub(crate) origin: Option<&'a str>,
pub(crate) origin: Option<Origin<'a>>,
pub(crate) line_start: usize,
pub(crate) source: &'a str,
pub(crate) markers: Vec<T>,
Expand Down Expand Up @@ -200,15 +200,20 @@ impl<'a, T: Clone> Snippet<'a, T> {

/// The location of the [`source`][Self::source] (e.g. a path)
///
/// If only a location is provided (i.e. a `String`) then the rest of the
/// [`Origin`] is inferred (e.g. line and column numbers). To adjust line
/// numbers, consider using [`Snippet::line_start`] instead as it will also
/// adjust line numbers for the [`Snippet::source`].
///
/// <div class="warning">
///
/// Text passed to this function is considered "untrusted input", as such
/// all text is passed through a normalization function. Pre-styled text is
/// not allowed to be passed to this function.
///
/// </div>
pub fn origin(mut self, origin: &'a str) -> Self {
self.origin = Some(origin);
pub fn origin(mut self, origin: impl Into<Origin<'a>>) -> Self {
self.origin = Some(origin.into());
self
Comment on lines +215 to 217
Copy link
Contributor

Choose a reason for hiding this comment

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

Not too sure how this makes it clearer.

Also, wht does uit mean to specify line, char_column, or primary on an Origin in a Snippet?

Copy link
Member Author

Choose a reason for hiding this comment

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

is_primary is respected unless the Origin is the "primary origin" for a Group (I think).

line is respected if it is set.

char_column is respected if line is set; if line is not set, it is overridden.

}

Expand Down Expand Up @@ -375,8 +380,16 @@ impl<'a> Patch<'a> {
}
}

/// The location of the [`Snippet`] (e.g. a path)
#[derive(Clone, Debug)]
/// The location of the [`Snippet`] (e.g. a path).
///
/// This should be used if you want to set the line number and column
/// explicitly for a [`Snippet`], or if you need to render a location without
/// an accompanying [`Snippet`].
Comment on lines +383 to +387
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The location of the [`Snippet`] (e.g. a path).
///
/// This should be used if you want to set the line number and column
/// explicitly for a [`Snippet`], or if you need to render a location without
/// an accompanying [`Snippet`].
/// The referenced location (e.g. a path)
///
/// If you have source available, see instead [`Snippet`]

///
/// Note: `line` is always respected if set, but `char_column` is only
/// respected if `line` has been set. `primary` is respected unless the origin
/// is the first one in a [`Group`], in which case it is ignored.
Comment on lines +389 to +391
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like good context to put on Origin::char_column

/// <div class="warning">
///
/// This will only be respected if [`Origin::line]` is also set
///
/// </div>

#[derive(Clone, Debug, Eq, PartialEq)]
pub struct Origin<'a> {
pub(crate) origin: &'a str,
pub(crate) line: Option<usize>,
Expand Down Expand Up @@ -412,17 +425,42 @@ impl<'a> Origin<'a> {
/// Set the default column to display
///
/// Otherwise this will be inferred from the primary [`Annotation`]
///
/// <div class="warning">
///
/// When [`Origin`] is passed into [`Snippet::origin`], `char_column` is
/// only be respected if [`Origin::line`] is also set.
///
/// </div>
Comment on lines +428 to +434
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait,. isn't this always applicable, not just Snippet::origin?

pub fn char_column(mut self, char_column: usize) -> Self {
self.char_column = Some(char_column);
self
}

/// <div class="warning">
///
/// When [`Origin`] is passed into [`Snippet::origin`], `primary` is
/// respected as long as the first [`Origin`] in a [`Group`].
///
/// </div>
pub fn primary(mut self, primary: bool) -> Self {
self.primary = primary;
self
}
}

impl<'a> From<&'a str> for Origin<'a> {
fn from(origin: &'a str) -> Self {
Self::new(origin)
}
}

impl<'a> From<&'a String> for Origin<'a> {
fn from(origin: &'a String) -> Self {
Self::new(origin)
}
}

fn newline_count(body: &str) -> usize {
#[cfg(feature = "simd")]
{
Expand Down