refactor: Use Origin for Snippet::origin #221
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Let me know what you think of the docs I added
There was a problem hiding this comment.
But when would you use Snippet::origin(Origin')?
| pub fn origin(mut self, origin: impl Into<Origin<'a>>) -> Self { | ||
| self.origin = Some(origin.into()); | ||
| self |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/snippet.rs
Outdated
| 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) | ||
| } | ||
| } | ||
|
|
||
| impl<'a> Origin<'a> { |
There was a problem hiding this comment.
nit: I tend to put trait impls after the regular functions as they tend to be more of the focus for a type
src/snippet.rs
Outdated
| /// If just a location is passed (i.e. a string) in the line and column | ||
| /// numbers are automatically inferred. If you want to explicitly set the | ||
| /// line and column you can create an [`Origin`] and pass it in. | ||
| /// |
There was a problem hiding this comment.
| /// If just a location is passed (i.e. a string) in the line and column | |
| /// numbers are automatically inferred. If you want to explicitly set the | |
| /// line and column you can create an [`Origin`] and pass it in. | |
| /// | |
| /// 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`] |
| /// 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. |
There was a problem hiding this comment.
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>
src/snippet.rs
Outdated
| /// This should be used if you want to display the file details by | ||
| /// itself, or you want to set the line number and column explicitly. | ||
| /// Otherwise, it is better to use [`Snippet::origin`]. |
There was a problem hiding this comment.
This should be clarified that you mean that this is a parallel item to Snippet (can't remember all of the terms and not digging in to provide a suggestion atm)
There was a problem hiding this comment.
I was referring to [Element::Origin]
Muscraft
force-pushed
the
use-origin
branch
3 times, most recently
from
30d349a to
960ef89
Compare
| /// | ||
| /// <div class="warning"> | ||
| /// | ||
| /// When [`Origin`] is passed into [`Snippet::origin`], `char_column` is | ||
| /// only be respected if [`Origin::line`] is also set. | ||
| /// | ||
| /// </div> |
There was a problem hiding this comment.
Wait,. isn't this always applicable, not just Snippet::origin?
| /// 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`]. |
There was a problem hiding this comment.
| /// 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`] |
| /// A source view [`Element`] in a [`Group`] | ||
| #[derive(Clone, Debug)] | ||
| pub struct Snippet<'a, T> { |
There was a problem hiding this comment.
| /// 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> { |
This partially addresses #208 by making
Snippet::originuseOrigininternally. This makes the distinction a little clearer: you should useSnippet::originif you are using aSnippetandOriginon its own if you need the file details.