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

String template curly braces bugfix #427

Merged
merged 12 commits into from
Oct 27, 2020

Conversation

aktsay6
Copy link
Collaborator

@aktsay6 aktsay6 commented Oct 19, 2020

What's done:

  • Fixed bug

This pull request closes #401

### What's done:
  * Fixed bug
@aktsay6 aktsay6 added the bug Something isn't working label Oct 19, 2020
@aktsay6 aktsay6 requested review from petertrr and kentr0w October 19, 2020 10:14
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #427 into master will decrease coverage by 0.05%.
The diff coverage is 62.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #427      +/-   ##
============================================
- Coverage     81.94%   81.89%   -0.06%     
- Complexity     1445     1448       +3     
============================================
  Files            70       70              
  Lines          3617     3623       +6     
  Branches       1155     1158       +3     
============================================
+ Hits           2964     2967       +3     
  Misses          202      202              
- Partials        451      454       +3     
Flag Coverage Δ Complexity Δ
#unittests 81.89% <62.50%> (-0.06%) 1448.00 <6.00> (+3.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...n/diktat/ruleset/rules/StringTemplateFormatRule.kt 85.29% <62.50%> (-7.57%) 12.00 <6.00> (+3.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28dfb9...07c5f77. Read the comment docs.

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
@@ -22,6 +21,7 @@ class StringTemplateRuleWarnTest : LintTestBase(::StringTemplateFormatRule) {
"""
|class Some {
| val template = "${'$'}{::String} ${'$'}{asd.moo()}"
| val some = "${'$'}{foo as Foo}"
Copy link
Member

@orchestr7 orchestr7 Oct 20, 2020

Choose a reason for hiding this comment

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

not much tests for a regression

### What's done:
  * Fixed bugs
|}
""".trimMargin(),
LintError(2, 20, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{a}", true),
LintError(3, 16, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{1.0}", true)
LintError(3, 16, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{1.0}", true),
LintError(4, 19, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{1}", true)
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean that braces can be omitted around dollar sign? Then you must ensure that your fixer escapes it for single-quoted strings and doesn't fix ${'$'} for triple-quoted strings (no way to escape $ there)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it was meant that braces can be omitted around integers or floats

Copy link
Member

@petertrr petertrr Oct 21, 2020

Choose a reason for hiding this comment

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

But they can't - in your example it's not a part of template, these braces are just regular characters inside the string. ${'$'} is a template, and {1.0} that goes after is a regular part of string.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this warning complains about $, not 1? Then it makes sense.. Do you have tests that ${'$'} is considered valid in triple-quoted strings?

### What's done:
  * Fixed bugs
### What's done:
  * Added test
|}
""".trimMargin(),
LintError(2, 20, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{a}", true),
LintError(3, 16, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{1.0}", true)
LintError(3, 16, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{1.0}", true),
LintError(4, 19, ruleId, "${Warnings.STRING_TEMPLATE_CURLY_BRACES.warnText()} ${'$'}{1}", true)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, this warning complains about $, not 1? Then it makes sense.. Do you have tests that ${'$'} is considered valid in triple-quoted strings?

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

lgtm

@aktsay6 aktsay6 merged commit 2d9e88e into master Oct 27, 2020
@aktsay6 aktsay6 deleted the bugfix/string-template-curly-braces(#401) branch October 27, 2020 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False-positive STRING_TEMPLATE_CURLY_BRACES
3 participants