Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

change error type for trailing commas, fixes #352 #353

Merged
merged 3 commits into from
Sep 9, 2017

Conversation

fmoor
Copy link
Contributor

@fmoor fmoor commented Sep 1, 2017

NOTE: There is one small defect with this patch. The line and column numbers that the error message reports match the position of the closing brace after the trailing comma and not the position of the trailing comma.

I plant to continue searching for the logic that determines the line and column numbers. If someone wants to speed things up and point me towards the relevant code it would be much appreciated.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I would go a step further and add an error variant that specifically means trailing comma. "Trailing characters" is pretty ambiguous.

@fmoor
Copy link
Contributor Author

fmoor commented Sep 1, 2017

There is still the problem of reporting misleading line and column numbers.

@fmoor
Copy link
Contributor Author

fmoor commented Sep 1, 2017

@dtolnay why are there suddenly a bunch of clippy errors in code I haven't touched? Should I make all of the suggested changes?

@dtolnay
Copy link
Member

dtolnay commented Sep 1, 2017

Clippy adds new lints occasionally. I filed #354 to follow up.

@fmoor fmoor force-pushed the feature/trailing_comma branch from 525afcc to 1b02cfe Compare September 6, 2017 00:26
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay dtolnay merged commit adade33 into serde-rs:master Sep 9, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants