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.

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

Add a Source::with_path method to set the path on a Source #3941

Merged
merged 3 commits into from
Aug 3, 2024
Merged

Add a Source::with_path method to set the path on a Source #3941

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b43c044
Add a Source::with_path method to set the path on a Source
hansl Aug 1, 2024
b31f4f0
.
hansl Aug 1, 2024
f9ff7c0
Align the doc to other with_... methods
hansl Aug 2, 2024
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
8 changes: 8 additions & 0 deletions core/parser/src/source/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,14 @@ impl<'path, R: Read> Source<'path, UTF8Input<R>> {
}

impl<'path, R> Source<'path, R> {
/// Add a path to the current [`Source`] instance.
Copy link
Member

Choose a reason for hiding this comment

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

nit: doc might be a bit unclear wording wise. As it's not adding the path to the current Source, but is creating a new Source from the combination.

Suggested change
/// Add a path to the current [`Source`] instance.
/// Returns a new source from the current [`Source`] instance combined with the provided `Path`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes ownership of the source, so it's used like any with_ method. I can use the same wording as JsError::with_message; Sets the path of this Source.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I've been dealing with different with_ methods recently with temporal 😆 I guess I'm a bit curious about taking self vs. mut self in this instance.

Basically, if I'm reading this correctly, this method could probably be:

// ... impl block

    pub fn with_path(mut self, new_path: &'path Path) -> Self {
        let _ = self.path.insert(new_path);
        self
    }

// cont ...

I think the above tends to be more in line with convention, and we also aren't potentially dropping the previous self, granted maybe rustc optimizes that out and it doesn't matter that much.

Copy link
Member

@jedel1043 jedel1043 Aug 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@nekevss Uhh I think your method doesn't work in general because it doesn't change the lifetime of Source to be the same as the lifetime of Path. It would (probably) work with lifetimes that are sub-lifetimes of 'path, but we don't want to add that arbitrary restriction.

Copy link
Member

Choose a reason for hiding this comment

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

True. I think I'm getting caught up on a bit of a bikeshed around the name/behavior of this method.

😕 if we are destroying Source anyways, then wouldn't it be a bit clearer convention wise for the method to be a to_ (something like to_source_with_path)

Copy link
Member

Choose a reason for hiding this comment

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

I think using with_* methods that take self is pretty common e.g. builder types. I don't see a problem with using the same convention for this.

pub fn with_path(self, new_path: &Path) -> Source<'_, R> {
Source {
reader: self.reader,
path: Some(new_path),
}
}

/// Returns the path (if any) of this source file.
pub fn path(&self) -> Option<&'path Path> {
self.path
Expand Down
Loading