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

Handle special types correctly #287

Merged
merged 1 commit into from
Nov 29, 2019
Merged

Conversation

CreepySkeleton
Copy link
Collaborator

While thinking on #285 I suggested to use ::vec::Vec instead of Vec to prevent special casing, but a simple test proved me wrong. In fact, any path that ends with Vec<T> will be considered as Vec<T> which is wrong. The same goes for Option and bool.

I believe this is a bug and it should be fixed. This PR handles paths correctly, checking not only the last segment but also verifying that the said segment is the only one segment present, no leading ::.

Technically, this is a breaking change but I believe this is a bug and, furthermore, I really doubt someone out there has been relying on this behavior.

@CreepySkeleton CreepySkeleton force-pushed the handle_paths_correctly branch 4 times, most recently from 8e6621e to 65d1773 Compare November 28, 2019 04:34
@TeXitoi
Copy link
Owner

TeXitoi commented Nov 29, 2019

There is some cases when it could be used: to have something collectable that is not a vec, one can declare something that ends with Vec to trick structopt.

I don't think neither that someone rely on that, so let's merge it. At worst, we will revert it if we have some bug reports.

@CreepySkeleton CreepySkeleton merged commit d1a50bf into master Nov 29, 2019
@CreepySkeleton CreepySkeleton deleted the handle_paths_correctly branch November 29, 2019 17:03
@TeXitoi
Copy link
Owner

TeXitoi commented Nov 29, 2019

We forgot the changelog for this one.

@CreepySkeleton
Copy link
Collaborator Author

I'll remember to make sure it'll end up being put in changelog one way or another, don't you worry :)

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.

2 participants