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

fix(build): split path types in compile #721

Merged
merged 1 commit into from
Oct 20, 2021
Merged

fix(build): split path types in compile #721

merged 1 commit into from
Oct 20, 2021

Conversation

aquarhead
Copy link
Contributor

Same idea as tokio-rs/prost#496 (it should explain both motivation and solution)

@davidpdrsn
Copy link
Member

Could this be a breaking change?

@aquarhead
Copy link
Contributor Author

Yes, I believe it will break if includes was empty, like &[]. The workaround I used in prost is change to &[""], but I'd like to know if there's a better solution.

@davidpdrsn
Copy link
Member

Ah right yeah. Can't think of a way around that 🤔 std::env::join_paths has the same issue, although that does allow you to use turbofish. I see thats also how you wrote it initially here.

I personally not sure using impl AsRef<Path> is strictly better than regular generic bounds but I suppose an empty includes is quite a niche use-case, though I'm not sure. Never done it myself 🤷

@aquarhead
Copy link
Contributor Author

The problem is that both protos and includes are using the same generic parameter, so they need to be of same type to make the compiler happy, which can be unnecessary and manual type conversion code.

I guess a solution is to provide another compile_simple function that does not take includes? Or just document this case?

@davidpdrsn
Copy link
Member

Or just document this case?

I would be okay with that I think. @LucioFranco what do you think?

@LucioFranco
Copy link
Member

Yes, this is a breaking change so lets hold off on it until we go for 0.6. We can keep the PR open though since it will still work down the line.

@davidpdrsn davidpdrsn added this to the 0.6 milestone Jul 15, 2021
@davidpdrsn davidpdrsn changed the title Split path types in compile fix(build): split path types in compile Jul 16, 2021
@LucioFranco LucioFranco merged commit 53ecc1f into hyperium:master Oct 20, 2021
@aquarhead aquarhead deleted the path-generic branch October 20, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants