Skip to content

Add StringShouldBeRawString.kt rule - #5705

Merged
BraisGabin merged 3 commits into
detekt:mainfrom
atulgpt:fixes-5697
Feb 21, 2023
Merged

Add StringShouldBeRawString.kt rule#5705
BraisGabin merged 3 commits into
detekt:mainfrom
atulgpt:fixes-5697

Conversation

@atulgpt

@atulgpt atulgpt commented Jan 17, 2023

Copy link
Copy Markdown
Contributor

Fixes #5697

@github-actions github-actions Bot added the rules label Jan 17, 2023
@github-actions

github-actions Bot commented Jan 17, 2023

Copy link
Copy Markdown
Messages
📖 Thanks for adding a new rule to Detekt ❤️

Generated by 🚫 dangerJS against 2af4f64

@codecov

codecov Bot commented Jan 17, 2023

Copy link
Copy Markdown

Codecov Report

Merging #5705 (2af4f64) into main (0f689b3) will increase coverage by 84.46%.
The diff coverage is 84.48%.

@@             Coverage Diff             @@
##             main    #5705       +/-   ##
===========================================
+ Coverage        0   84.46%   +84.46%     
- Complexity      0     3753     +3753     
===========================================
  Files           0      544      +544     
  Lines           0    12763    +12763     
  Branches        0     2236     +2236     
===========================================
+ Hits            0    10780    +10780     
- Misses          0      865      +865     
- Partials        0     1118     +1118     
Impacted Files Coverage Δ
...osch/detekt/rules/style/StringShouldBeRawString.kt 84.21% <84.21%> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <100.00%> (ø)
...n/io/gitlab/arturbosch/detekt/api/ConsoleReport.kt 100.00% <0.00%> (ø)
...n/io/github/detekt/report/sarif/RuleDescriptors.kt 30.43% <0.00%> (ø)
...io/github/detekt/tooling/internal/PluginsHolder.kt 100.00% <0.00%> (ø)
...tlab/arturbosch/detekt/core/reporting/Colorizer.kt 78.57% <0.00%> (ø)
...bosch/detekt/api/internal/CommaSeparatedPattern.kt 100.00% <0.00%> (ø)
...arturbosch/detekt/generator/printer/RulePrinter.kt 96.66% <0.00%> (ø)
...rturbosch/detekt/core/settings/PropertiesFacade.kt 100.00% <0.00%> (ø)
...ub/detekt/psi/internal/FullQualifiedNameGuesser.kt 87.50% <0.00%> (ø)
... and 535 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@3flex

3flex commented Jan 17, 2023

Copy link
Copy Markdown
Member

Couple of notes:

  • rule name should reference "multiline" in the name, not just "raw string"
  • I think there should be an option to ignore single lines with \n. Sometimes it's simpler to have a string like "line 1\nline 2", than to convert to a multiline string (and remember we have other rules that relate to formatting multiline strings and enforcing use of trimIndent and trimMargin). All of a sudden there's a bunch of extra things to get "right". Or there could be an option for single lines for max number of new line escape characters which triggers the violation

@BraisGabin

Copy link
Copy Markdown
Member

rule name should reference "multiline" in the name, not just "raw string"

The second example on the issue doesn't talk about multi-line. A regex is a good example of a single line raw string that is better than a normal string.

@3flex

3flex commented Jan 18, 2023

Copy link
Copy Markdown
Member

Good point. I only looked at this very quickly, but the issue being fixed is "Rule for encouraging MultilineRawString". The proposed rule is a bit different.

Agree that this proposal makes sense as is, so ignore my comment :)

Rename fun to getStringSequenceExcludingRawString

@BraisGabin BraisGabin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks good :)

Comment on lines +84 to +86
if (maxEscapedCharacterCount == Int.MAX_VALUE) {
return
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the user doesn't want this rule s/he should disable it. We don't need to have two different ways to disable it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. But currently if we set Int.MAX_VALUE in the detekt.yml the working of rule breaks. BTW @BraisGabin is there any sanitisation on user input through detekt.yml?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Int.MAX_VALUE doesn't seems like a sane value. We shouldn't care about that. If they would like the max value of a long it wouldn't work either 🤷.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And there is little sanitation on that regard as far as I know.

@BraisGabin BraisGabin added this to the 1.23.0 milestone Jan 18, 2023
Remove corresponding Int tests
Make extension parameter non null
@atulgpt

atulgpt commented Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

Hi @BraisGabin is this PR good for merge? Or some changes are required? cc @cortinico @TWiStErRob for their inputs as well

@BraisGabin

Copy link
Copy Markdown
Member

This is good to go! Sorry for the delay :)

@BraisGabin
BraisGabin merged commit 576a3f7 into detekt:main Feb 21, 2023
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notable changes Marker for notable changes in the changelog rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rule for encouraging MultilineRawString

5 participants