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.

Sign up for GitHub

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

Jump to bottom

Use multi line format for list options in config #3827

Merged
merged 4 commits into from
Jun 27, 2021
Merged

Use multi line format for list options in config #3827

merged 4 commits into from
Jun 27, 2021

Conversation

marschwar
Copy link
Contributor

This closes #3671 and generates default list option in multi line format.

@codecov
Copy link

codecov bot commented May 24, 2021 •
edited
Loading

Codecov Report

Merging #3827 (b760a08) into main (44c1ef0) will increase coverage by 0.00%.
The diff coverage is 90.90%.

❗ Current head b760a08 differs from pull request most recent head ae976a3. Consider uploading reports for the commit ae976a3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #3827   +/-   ##
=========================================
  Coverage     83.53%   83.54%           
- Complexity     3120     3125    +5     
=========================================
  Files           456      456           
  Lines          8996     9011   +15     
  Branches       1753     1757    +4     
=========================================
+ Hits           7515     7528   +13     
  Misses          565      565           
- Partials        916      918    +2
Impacted Files Coverage Δ
.../io/gitlab/arturbosch/detekt/generator/out/Yaml.kt 70.27% <77.77%> (+1.30%) ⬆️
...bosch/detekt/generator/collection/Configuration.kt 100.00% <100.00%> (ø)
...ekt/generator/printer/rulesetpage/ConfigPrinter.kt 92.10% <100.00%> (-0.76%) ⬇️

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 44c1ef0...ae976a3. Read the comment docs.

@cortinico cortinico added this to the 1.18.0 milestone May 24, 2021
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label May 24, 2021
@marschwar marschwar force-pushed the always-use-multi-line-lists branch from 59f4202 to 158ad0f Compare May 25, 2021 13:04
@marschwar marschwar force-pushed the always-use-multi-line-lists branch from 86d694b to 27ea847 Compare June 6, 2021 10:30
schalkms
schalkms reviewed Jun 26, 2021
View reviewed changes
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Is there anything else left to do besides handling the merge conflict?

detekt-core/src/main/resources/default-detekt-config.yml Outdated Show resolved Hide resolved
detekt-core/src/main/resources/default-detekt-config.yml Show resolved Hide resolved
detekt-core/src/main/resources/default-detekt-config.yml Show resolved Hide resolved
detekt-generator/src/test/kotlin/io/gitlab/arturbosch/detekt/generator/out/YamlSpec.kt Outdated Show resolved Hide resolved
@marschwar marschwar force-pushed the always-use-multi-line-lists branch from c5b3e76 to 6f0d0c4 Compare June 26, 2021 17:35
@marschwar
Copy link
Contributor Author

Is there anything else left to do besides handling the merge conflict?

From my point of view this is ready. There may be objections to making such a change as part of a minor version though.

chao2zhang
chao2zhang approved these changes Jun 27, 2021
View reviewed changes
...t-generator/src/test/kotlin/io/gitlab/arturbosch/detekt/generator/util/RuleSetPageCreator.kt
Configuration(
name = "rulesetconfig3",
description = "description rulesetconfig2",
defaultValue = "['first', 'se*cond']",
Copy link
Member

Choose a reason for hiding this comment

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

Not an issue, but just curious that if the asterisk is intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is. The asterisk has a special meaning in yaml and the value has to be enclosed in parentheses.

@chao2zhang chao2zhang mentioned this pull request Jun 27, 2021
Closed
@schalkms schalkms merged commit 7b7f76c into detekt:main Jun 27, 2021
@schalkms
Copy link
Member

There may be objections to making such a change as part of a minor version though.

The next version is v1.18 after v1.17.1. Hence I think it's okay.

@chao2zhang chao2zhang mentioned this pull request Jun 29, 2021
Merged
@marschwar marschwar mentioned this pull request Jul 5, 2021
Merged
@marschwar marschwar deleted the always-use-multi-line-lists branch July 27, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schalkms schalkms schalkms left review comments

@chao2zhang chao2zhang chao2zhang approved these changes

@BraisGabin BraisGabin BraisGabin approved these changes

Assignees
No one assigned
Labels
notable changes Marker for notable changes in the changelog
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.

Use dynamic list format for default-detekt-config.yml
5 participants
@marschwar @schalkms @BraisGabin @chao2zhang @cortinico