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

use same behaviour for valueOrNull as for valueOrDefault #2319

Conversation

robstoll
Copy link
Contributor

@robstoll robstoll commented Feb 5, 2020

  • implemented valueOrDefault based on valueOrNull per default
  • kept a different implementation for Test/YamlConfig

-fixes #2316

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.

Execution failed for task ':detekt-generator:verifyGeneratorOutput'.
959> The detekt documentation is not up-to-date. Please build detekt locally to update it and commit the changed files.

@schalkms
Copy link
Member

schalkms commented Feb 8, 2020

Thanks for tackling that! It looks good to me.

@robstoll robstoll force-pushed the #2316-config-valueOrDefault-based-on-valueOrNull branch from 30d2807 to 413ea6f Compare February 9, 2020 15:05
- implemented valueOrDefault based on valueOrNull per default
- kept a different implementation for Test/YamlConfig as the
  valueOrDefault does more than valueOrNull (it parses values), due to
  this, valueOrDefault needs to be overriden
  (and cannot be based on valueOrNull) in:
  - ConfigAware
  - CompositeConfig
@robstoll robstoll force-pushed the #2316-config-valueOrDefault-based-on-valueOrNull branch from 413ea6f to 127ff3a Compare February 9, 2020 15:09
@robstoll
Copy link
Contributor Author

robstoll commented Feb 9, 2020

@schalkms I have updated the documentation. Moreover, I had to change the PR slightly:
ConfigAware and CompositeConfig need to override valueOrDefault due to YamlConfig and TestConfig which implement an additional behaviour in valueOrDefault (LSP violation right in your face 😆).
I don't know the reasons behind the parsing in valueOrDefault and whether this could be moved into Config as well (so that all configs behave the same way) or not.

@codecov-io
Copy link

codecov-io commented Feb 9, 2020

Codecov Report

Merging #2319 into master will decrease coverage by 0.01%.
The diff coverage is 55.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2319      +/-   ##
============================================
- Coverage     81.87%   81.86%   -0.02%     
  Complexity     2119     2119              
============================================
  Files           348      348              
  Lines          6047     6049       +2     
  Branches       1108     1110       +2     
============================================
+ Hits           4951     4952       +1     
  Misses          512      512              
- Partials        584      585       +1
Impacted Files Coverage Δ Complexity Δ
...b/arturbosch/detekt/api/internal/FailFastConfig.kt 36.36% <0%> (ø) 5 <0> (ø) ⬇️
...io/gitlab/arturbosch/detekt/api/CompositeConfig.kt 66.66% <66.66%> (ø) 5 <3> (ø) ⬇️
...n/kotlin/io/gitlab/arturbosch/detekt/api/Config.kt 90% <75%> (-1.67%) 0 <0> (ø)

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 f5fd080...127ff3a. Read the comment docs.

@arturbosch arturbosch merged commit b017b77 into detekt:master Feb 16, 2020
@arturbosch arturbosch added this to the 1.6.0 milestone Feb 16, 2020
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.

FailFastConfig.valueOrNull should return specified value for active and maxIssues
4 participants