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

Add config delegate to public api #3891

Merged
merged 6 commits into from
Jul 18, 2021

Conversation

marschwar
Copy link
Contributor

This is a part of #3670.

I was going to mark ThresholdRule and LazyRegex as deprecated but then I did not know what to suggest to users on how to replace it unless making the config property part of the public api. I think the "nicest" way is to mark the functions whith @UnstableApi but then almost all native rules would have to opt in which also seems weird.

Please let me know your opinion on how to move forward with this.

@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #3891 (d25b52f) into main (40abc3f) will decrease coverage by 0.00%.
The diff coverage is 97.56%.

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

@@             Coverage Diff              @@
##               main    #3891      +/-   ##
============================================
- Coverage     83.44%   83.43%   -0.01%     
  Complexity     3149     3149              
============================================
  Files           456      456              
  Lines          9014     9015       +1     
  Branches       1754     1754              
============================================
  Hits           7522     7522              
- Misses          565      566       +1     
  Partials        927      927              
Impacted Files Coverage Δ
...otlin/io/github/detekt/custom/SpekTestDiscovery.kt 65.45% <ø> (ø)
...otlin/io/gitlab/arturbosch/detekt/api/LazyRegex.kt 0.00% <0.00%> (ø)
...n/io/gitlab/arturbosch/detekt/api/ThresholdRule.kt 0.00% <ø> (ø)
...detekt/formatting/wrappers/ArgumentListWrapping.kt 100.00% <ø> (ø)
...urbosch/detekt/formatting/wrappers/FinalNewline.kt 100.00% <ø> (ø)
...bosch/detekt/formatting/wrappers/ImportOrdering.kt 100.00% <ø> (ø)
...turbosch/detekt/formatting/wrappers/Indentation.kt 100.00% <ø> (ø)
...ch/detekt/formatting/wrappers/MaximumLineLength.kt 100.00% <ø> (ø)
...etekt/formatting/wrappers/ParameterListWrapping.kt 100.00% <ø> (ø)
...rbosch/detekt/rules/complexity/ComplexCondition.kt 88.09% <ø> (ø)
... and 65 more

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 40abc3f...3e4183f. Read the comment docs.

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.

Can you please elaborate on the solution?
What are the pros and cons of this solution?
For which parts of the implementation would you like to have feedback?

@marschwar
Copy link
Contributor Author

marschwar commented Jun 24, 2021

Sorry, I should have been more specific. The questions are less about the actual implementation but rather about the general way forward.

  • Should the config property delegates become part of the public api?
  • Should the @Configuration become part of the public api?
  • Should the new parts of the api be marked with @UnstableApi?
  • Do we agree that the ThresholdRule and LazyRegeg should be marked as deprecated and will be removed with detekt 2.0?

@schalkms
Copy link
Member

Should the config property delegates become part of the public api?
Should the @configuration become part of the public api?

I'm not sure here. Do we really want to publish those property delegates and annotations? We would then need to keep them stable as part of the API.

Should the new parts of the api be marked with @UnstableApi?

+1

Do we agree that the ThresholdRule and LazyRegeg should be marked as deprecated and will be removed with detekt 2.0?

+1

@chao2zhang
Copy link
Member

My answers would be as follows:

  • Should the config property delegates become part of the public API? - Yes
  • Should the @configuration become part of the public API? - Yes
  • Should the new parts of the api be marked with @UnstableApi? - Probably no, since we have migrated all detekt rules to @configuration.
  • Do we agree that the ThresholdRule and LazyRegex should be marked as deprecated and will be removed with detekt 2.0? - Yes

@marschwar
Copy link
Contributor Author

I am less sure about ActiveByDefault, @Configuration and @RequiresTypeResolution as they are only used to generate the config file and documentation. I do not see any benefit right now to expose those.

@marschwar marschwar changed the title [RFC] Add config delegate to public api Add config delegate to public api Jun 25, 2021
@chao2zhang chao2zhang added this to the 1.18.0 milestone Jul 1, 2021
@chao2zhang
Copy link
Member

@schalkms since you have reviewed this PR, would you mind taking another look again?

@schalkms
Copy link
Member

@chao2zhang fine for me. Merging this PR helps to close the linked issue as well, since all TODOs are now done.👍

@schalkms
Copy link
Member

Excuse me please, I overlooked the latest changes in this PR whilst checking my GitHub notifications.

@chao2zhang chao2zhang merged commit c5850c4 into detekt:main Jul 18, 2021
@marschwar marschwar deleted the remove-threshold-rule branch July 19, 2021 11:07
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Aug 5, 2021
@cortinico
Copy link
Member

@marschwar Would you be able to write some documentation on this API (both the configuration delegate and all the new annotations) so I can add it to the release notes for 1.18.0?

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants