Skip to content

File inclusion with an explicit destination path#11

Open
LukeMathWalker wants to merge 7 commits intomgattozzi:mainfrom
LukeMathWalker:file-inclusion
Open

File inclusion with an explicit destination path#11
LukeMathWalker wants to merge 7 commits intomgattozzi:mainfrom
LukeMathWalker:file-inclusion

Conversation

@LukeMathWalker
Copy link
Copy Markdown

As discussed in #9, this PR changes the behaviour of the include attribute:

  • #[assay(include = ["folder/my_file.rs"]) will copy the file into the root folder of the test working directory (i.e. file.rs);
  • #[assay(include = [("folder/my_file.rs", "folder/my_renamed_file.rs"]) will instead use the specified destination path.

I have a few questions on the implementation that I've left as TODO comment in the code. I'd appreciate your input there.

Closes #9

src/lib.rs Outdated
}

pub fn include(&self, path: impl AsRef<Path>) -> Result<(), Box<dyn Error>> {
pub fn include(&self, source_path: impl AsRef<Path>, destination_path: Option<&str>) -> Result<(), Box<dyn Error>> {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

An alternative here would be to add a generic parameter S: AsRef<Path> and tie both inputs to use S.
We cannot have destination_path: Option<impl AsRef<Path>> because it causes an inference error when the path is omitted.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@LukeMathWalker would it be possible to do this so they can be separate types or is the inference breaking here?

pub fn include(&self, source_path: S, destination_path: Option<D>)
where S: AsRef<Path>,
      D: AsRef<Path>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That still failed, but I managed to work around the inference issue by forcing D to be PathBuf in the code generated by the proc macro for the None case.

Copy link
Copy Markdown
Owner

@mgattozzi mgattozzi left a comment

Choose a reason for hiding this comment

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

This is great! 🤩 I think we can pass off on parsing errors for now as I don't have a great idea of what I want there, but overall these changes are fantastic! Let's take out the TODO around the panic and if the double generic works let's do that!

Luca Palmieri added 3 commits April 1, 2022 10:51
…ce error by using the turbofish to force D=PathBuf when the destination path is unspecified.
@LukeMathWalker
Copy link
Copy Markdown
Author

Thanks for the review!
TODOs are gone, the generic issue has been solved and the panics have been polished. It should be ready for a second look 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Specify a target filepath for included files

2 participants