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

Introduce end-2-end codegen tests & prepare ground for issue #39 #42

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

magnet
Copy link
Contributor

@magnet magnet commented Mar 8, 2020

Draft, early review welcome @cpcloud @lovesegfault

I'm not sure src is the best place to put the expected source code, especially because it is Rust and it may be confused with code that is actually ran (a different extension might help, but breaks syntax highlighting from IDEs, which is appreciated), and tests is usually reserved for integration tests (where one can call into the crate as a 3rd party).

Ideas welcome!

Copy link
Collaborator

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

This approach seems reasonable to me. Looks like the trybuild crate uses the tests directory for storing expected output files. I wouldn't be opposed to using tests either.


// Split the String per lines to make debugging/diffing
// viable (it also improves diff performance at test time)
let v : Vec<_> = formatted.lines().map(ToString::to_string).collect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be assigned to a variable?

Copy link
Contributor Author

@magnet magnet Mar 9, 2020

Choose a reason for hiding this comment

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

collect needs to know the required type, so either this variable or using the turbofish operator. I'm partial to variables because it's usually more explicit/readable, expresses the intent better. (This is needed to let the macro return a Vec)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure I just mean as opposed to turbofish, which I find easier for reading an expression from start to finish without having to go back some number of lines to remember what's being collected. However I don't feel too strongly about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However I don't feel too strongly about it.

Same 🙂. My main grippe with turbofish is that sometimes functions take more than one generic, and there's more to annotate than just one type even though we're interested in only one of them: that's not the case for collect so I'm fine with both.

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.

None yet

2 participants