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 name that return name after backticks #5719

Merged
merged 2 commits into from
Feb 23, 2023
Merged

Use name that return name after backticks #5719

merged 2 commits into from
Feb 23, 2023

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented Jan 22, 2023

Fixes #5531

@github-actions github-actions bot added the rules label Jan 22, 2023
@codecov
Copy link

codecov bot commented Jan 22, 2023 •
edited
Loading

Codecov Report

Merging #5719 (9362e1b) into main (2290ec8) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #5719      +/-   ##
============================================
- Coverage     84.61%   84.59%   -0.02%     
- Complexity     3787     3789       +2     
============================================
  Files           546      546              
  Lines         12918    12918              
  Branches       2268     2268              
============================================
- Hits          10930    10928       -2     
  Misses          862      862              
- Partials       1126     1128       +2
Impacted Files Coverage Δ
.../detekt/rules/naming/ConstructorParameterNaming.kt 88.88% <50.00%> (-5.56%) ⬇️

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

@atulgpt
Copy link
Contributor Author

atulgpt commented Feb 21, 2023

Hi @BraisGabin here can I do something to make the patch diff pass?

BraisGabin
BraisGabin reviewed Feb 22, 2023
View reviewed changes
Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I think that the configuration is not important for this case, right?

...g/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNamingSpec.kt Outdated Show resolved Hide resolved
...g/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNamingSpec.kt Outdated Show resolved Hide resolved
...g/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNamingSpec.kt Outdated Show resolved Hide resolved
...g/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNamingSpec.kt Outdated Show resolved Hide resolved
@BraisGabin
Copy link
Member

Hi @BraisGabin here can I do something to make the patch diff pass?

No, it's ok. Your are testing this code enough.

@atulgpt
Copy link
Contributor Author

atulgpt commented Feb 22, 2023

I think that the configuration is not important for this case, right?

Yes @BraisGabin as using is, when these as the variable name is valid code. Also we have separate rule to check unneccesary backticks

@BraisGabin BraisGabin enabled auto-merge (squash) February 22, 2023 09:28
@BraisGabin BraisGabin disabled auto-merge February 22, 2023 09:44
@BraisGabin BraisGabin enabled auto-merge (squash) February 22, 2023 09:45
auto-merge was automatically disabled February 22, 2023 21:00

Head branch was pushed to by a user without write access

@github-actions
Copy link

github-actions bot commented Feb 22, 2023 •
edited
Loading

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the Detekt release notes.

Generated by 🚫 dangerJS against 9362e1b

@atulgpt
Copy link
Contributor Author

atulgpt commented Feb 22, 2023

Hi, @BraisGabin previous check got failed due to the Stream not allowed rule. Now I have rebased my branch. Although codecov/patch still failing

@BraisGabin
Copy link
Member

Don't worry about that one.

@BraisGabin BraisGabin merged commit 718c7d5 into detekt:main Feb 23, 2023
@atulgpt atulgpt deleted the fixes-5531 branch February 23, 2023 20:23
@cortinico cortinico added this to the 1.23.0 milestone Feb 25, 2023
@cortinico
Copy link
Member

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the Detekt release notes.

Generated by 🚫 dangerJS against 9362e1b

Quick heads up that those warning should not be ignored, otherwise those PRs won't make it to the changelog, while in this case this one is touching the biz logic :)

@atulgpt
Copy link
Contributor Author

atulgpt commented Feb 25, 2023

Hi @cortinico, I think I can not set the milestone. Let me know if I need to address these warnings

@cortinico
Copy link
Member

Hi @cortinico, I think I can not set the milestone. Let me know if I need to address these warnings

It's more a reminder for @BraisGabin and other maintainers 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BraisGabin BraisGabin BraisGabin approved these changes

Assignees
No one assigned
Labels
rules
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

ConstructorParameterNaming accounting backticks as part of the parameter name
3 participants
@atulgpt @BraisGabin @cortinico