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

Now error messages highlight the error location #225

Merged
merged 2 commits into from
Aug 13, 2019
Merged

Now error messages highlight the error location #225

merged 2 commits into from
Aug 13, 2019

Conversation

CreepySkeleton
Copy link
Collaborator

A work on error reporting, as promised in #5 .

First of all, I must genuinely apologize for this long delay, the summer had happened to be more busy and eventful than I'd expected. I don't think something like that will happen this month, although you never know what's going to happen, don't you? Sorry again.

About PR:

It seems like the current (old) implementation drops Spans as soon as possible which was quite sensible: there was no way to use it so there was no need to carry needless stuff around. Worse yet: most of data structures are enums and one doesn't simply attach a piece of data to an enum. Basically, we have two ways:

  • first is to attach this data to every variant, like

    pub enum Ty {
        Bool(Span),
        Vec(Span),
        Option(Span),
        OptionOption(Span),
        OptionVec(Span),
        Other(Span),
    }

    Easy to see, this is extremely cumbersome. Despite this, I had to use this approach for some enums
    (or just one... can't remember), mostly because I was forced to by a trait interface.

  • second is to separate the kind and the metadata:

    pub struct Ty {
        span: Span,
        kind: TyKind
    }
    
    pub enum TyKind { Bool, Vec /* ... */ }

    The drawback here is there are a quite a lot of enums out here. I chose this approach, but slightly modified. There is struct Sp<T> in spanned module. It's essentially a wrapper around T which carries the span needed. For the sake of convince it implements Deref and some From traits.

Besides the Span problem mentioned, I've performed some refactoring, typically simplifying the code .

Most relevant changes are in src/*.rs files.

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 12, 2019

Thanks for the contribution. I'll try to be fast on the review, but no promise.

Don't be sorry! We give our free time on these project, and thus we don't have any obligation.

Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

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

Great job! I love it! Really a better programming experience!

Some minor remarks, feel free to address/respond/ignore, that's mergeable as is!

Thanks again!

structopt-derive/src/attrs.rs Outdated Show resolved Hide resolved
structopt-derive/src/parse.rs Outdated Show resolved Hide resolved
tests/ui/flatten_and_doc.stderr Show resolved Hide resolved
tests/ui/skip_without_default.rs Show resolved Hide resolved
tests/ui/structopt_empty_attr.stderr Outdated Show resolved Hide resolved
@TeXitoi TeXitoi merged commit 5dfa606 into TeXitoi:master Aug 13, 2019
@TeXitoi
Copy link
Owner

TeXitoi commented Aug 13, 2019

Thanks! Now I must find time to do #217 and then publish v0.3.

@CreepySkeleton CreepySkeleton deleted the nice_errors branch August 13, 2019 13:19
@CreepySkeleton
Copy link
Collaborator Author

You should consider to publish v0.2.19 so people who have to stick to v0.2.x for one reason or another could make use of this improvement. Contrary wise, you may want not to do that to encourage people to move to v0.3.x and publish 0.2.19 only if/when requested. Anyway, you should know better.

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 13, 2019

There is a lot to backport for that. Current master is already breaking 0.2.

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