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

Implement SA0002 (InvalidSettingsFile) #2032

Merged
merged 5 commits into from
Jan 12, 2016

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Jan 8, 2016

Fixes #2029

Screenshot of a report when the syntax error matches the one described in #2028:

image

@sharwell sharwell self-assigned this Jan 8, 2016
@sharwell sharwell added this to the 1.1.0 Beta 1 milestone Jan 8, 2016
@codecov-io
Copy link

Current coverage is 93.38%

Merging #2032 into master will increase coverage by +0.01% as of 2679bcb

@@            master   #2032   diff @@
======================================
  Files          561     563     +2
  Stmts        35587   35642    +55
  Branches      2284    2289     +5
  Methods                          
======================================
+ Hit          33228   33283    +55
- Partial        748     752     +4
+ Missed        1611    1607     -4

Review entire Coverage Diff as of 2679bcb

Powered by Codecov. Updated on successful CI builds.

Previously the test used the wrong form for verifying diagnostics without
a location.
@vweijsters
Copy link
Contributor

❓ Would it be possible to suppress the call stack dump, as it does not really provide any useful information?

@sharwell
Copy link
Member Author

sharwell commented Jan 9, 2016

@vweijsters It has the line and position numbers within the file where the problem occurred. It might not be exact, but it should be close. Also keep the following in mind:

  • The description is hidden by default. You have to expand the line in the error list to see them.
  • It's possible the stack trace could save time if/when issues are reported. In those cases, it's much easier to copy one that's provided than to explain to users how to attach the debugger and find out.

@sharwell sharwell removed their assignment Jan 9, 2016
@vweijsters
Copy link
Contributor

@sharwell The line and position numbers are in the exception message, so there is no need to have the stack trace for that. Besides that, the stack trace is probably not really useful for debugging, as it will only be dumped for JsonException.

@vweijsters
Copy link
Contributor

👍 Looks good otherwise

@sharwell
Copy link
Member Author

Updated to only show this:

image

@vweijsters
Copy link
Contributor

👍

sharwell added a commit that referenced this pull request Jan 12, 2016
Implement SA0002 (InvalidSettingsFile)
@sharwell sharwell merged commit bd2f647 into DotNetAnalyzers:master Jan 12, 2016
@sharwell sharwell deleted the fix-2029 branch January 12, 2016 19:53
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.

3 participants