Skip to content

Issue #15967: fix yield fails indentation in switch with forceStrictCondition#16131

Merged
nrmancuso merged 1 commit into
checkstyle:masterfrom
mohitsatr:yield-indent
Jan 29, 2025
Merged

Issue #15967: fix yield fails indentation in switch with forceStrictCondition#16131
nrmancuso merged 1 commit into
checkstyle:masterfrom
mohitsatr:yield-indent

Conversation

@mohitsatr

@mohitsatr mohitsatr commented Jan 8, 2025

Copy link
Copy Markdown
Member

fixes #15967

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/all-examples-in-one

@github-actions

github-actions Bot commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/all-examples-in-one

@github-actions

Copy link
Copy Markdown
Contributor

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/all-examples-in-one

@github-actions

Copy link
Copy Markdown
Contributor

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/all-examples-in-one

@github-actions

Copy link
Copy Markdown
Contributor

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/all-examples-in-one

@github-actions

Copy link
Copy Markdown
Contributor

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/Example2

@github-actions

Copy link
Copy Markdown
Contributor

@mohitsatr

mohitsatr commented Jan 13, 2025

Copy link
Copy Markdown
Member Author

@romani please check out regression report #16131 (comment)

basically it resolves the original problem for which issue is raised. there is just one false positive case
i have been playing with it and looks like it requires more than a simple code change as there are more diffs in subsequent reports. I think we should raise separate issue for that case. what do you think?

other report : #16131 (comment)
#16131 (comment)
problem arises when switch appears as a condition in if

if (switch(today) {
   . .. .
    }
)

ps: please ignore diff reports which are not linked in this comment

@mohitsatr

Copy link
Copy Markdown
Member Author

Github, generate report for Indentation/all-examples-in-one

@github-actions

Copy link
Copy Markdown
Contributor

@romani romani 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.

items:

@romani romani 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.

good to merge !!

thanks a lot !

@nrmancuso

Copy link
Copy Markdown
Contributor

@romani please reply to comment at #16131 (comment)

@mohitsatr please rebase on the latest master branch

@nrmancuso nrmancuso assigned romani and unassigned nrmancuso Jan 25, 2025
@romani

romani commented Jan 25, 2025

Copy link
Copy Markdown
Member

yes, lets create separate issue on this case, we can improve lateron on this.

@mohitsatr

Copy link
Copy Markdown
Member Author

lets create separate issue on this case

That case existed even before the new changes. With these changes, we’ve just replaced one incorrect message with another. We can leave it as it is for now. If anyone encounters a problem, they can raise the issue. what do you think ?

@romani

romani commented Jan 26, 2025

Copy link
Copy Markdown
Member

I am ok to not create issue , we have already a lot on this Check. And we had bad calculation of expected intendtation and after this update it becomes even more bad, but for user main problem is violation existence, not message.

@romani

romani commented Jan 26, 2025

Copy link
Copy Markdown
Member

Github, generate report for Indentation/all-examples-in-one

@github-actions

Copy link
Copy Markdown
Contributor

@nrmancuso nrmancuso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let’s do it

@nrmancuso nrmancuso merged commit 977c425 into checkstyle:master Jan 29, 2025
@mohitsatr mohitsatr deleted the yield-indent branch January 29, 2025 06:27
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.

yield fails Indentation check in switch expressions

3 participants