Skip to content

String format args#303

Merged
viniciushana merged 2 commits intocode-cracker:masterfrom
giggio:string-format-args
Mar 31, 2015
Merged

String format args#303
viniciushana merged 2 commits intocode-cracker:masterfrom
giggio:string-format-args

Conversation

@giggio
Copy link
Copy Markdown
Member

@giggio giggio commented Mar 31, 2015

Closes #116.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 31, 2015

Coverage Status

Coverage increased (+0.0%) to 89.98% when pulling 45d4be3 on giggio:string-format-args into 97c1a25 on code-cracker:master.

@viniciushana viniciushana self-assigned this Mar 31, 2015
viniciushana
viniciushana reviewed Mar 31, 2015
View reviewed changes
Comment thread src/CSharp/CodeCracker/Usage/StringFormatArgsAnalyzer.cs
Copy link
Copy Markdown
Contributor

@viniciushana viniciushana Mar 31, 2015

Choose a reason for hiding this comment

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

While I know it's a matter of personal style and the way that we have to do stuff with Roslyn doesn't help, I'd love to see some of the more complicated validations here extracted to meaningful methods. I think that block full of complex conditions gets quite overwhelming sometimes.

Copy link
Copy Markdown
Member Author

@giggio giggio Mar 31, 2015

Choose a reason for hiding this comment

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

I usually do that when the complexity gets high, but 16 lines on a method is not that bad. And this code is pretty much self explanatory, IMO.

Copy link
Copy Markdown
Contributor

@viniciushana viniciushana Mar 31, 2015

Choose a reason for hiding this comment

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

My comment was not much on the line count of the method, instead it's about the complexity of some calls. Anyway, it's more of a personal thing so it doesn't matter so much.

@viniciushana
Copy link
Copy Markdown
Contributor

viniciushana commented Mar 31, 2015

Overall it looks pretty good, and besides my inline comments, I missed tests with string interpolation. Are they supposed to emit diagnostics? How should this analyzer behave with them? There was code that seemed to ignore interpolated strings but, again, this is only an assumption since there was no test to check for this.

@giggio giggio force-pushed the string-format-args branch from 45d4be3 to fb7702a Compare March 31, 2015 15:01
@giggio
Copy link
Copy Markdown
Member Author

giggio commented Mar 31, 2015

@viniciushana String interpolation test added on line 110 of the test class.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 31, 2015

Coverage Status

Coverage increased (+0.02%) to 90.0% when pulling fb7702a on giggio:string-format-args into 97c1a25 on code-cracker:master.

@viniciushana
Copy link
Copy Markdown
Contributor

viniciushana commented Mar 31, 2015

👍

viniciushana added a commit that referenced this pull request Mar 31, 2015
@viniciushana
Merge pull request #303 from giggio/string-format-args
5a3b48b 
String format args
@viniciushana viniciushana merged commit 5a3b48b into code-cracker:master Mar 31, 2015
@giggio giggio deleted the string-format-args branch March 31, 2015 16:54
@viniciushana viniciushana removed their assignment Apr 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

3 - Done

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Check arguments in String.Format

3 participants

@giggio @coveralls @viniciushana