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

BooleanExpressionRule fixes #1295

Merged
merged 10 commits into from
May 25, 2022
Merged

BooleanExpressionRule fixes #1295

merged 10 commits into from
May 25, 2022

Conversation

nulls
Copy link
Member

@nulls nulls commented May 24, 2022

Which rule and warnings did you add?

This pull request closes #1284 and #961
BooleanExpressionRule:

  • substitution issues: A & B & C -> isBar & B & C -> isfooar & foo & C and isAdd && isAdded() -> A & Aed
  • negative expression: !isEmpty() -> A
  • ordering issue: isA && isB is isC -> isC && isA && isC

Actions checklist

  • Added tests on fixers

Fixme

@petertrr petertrr linked an issue May 24, 2022 that may be closed by this pull request
@nulls nulls self-assigned this May 24, 2022
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #1295 (ed49717) into master (7f116ac) will increase coverage by 0.03%.
The diff coverage is 90.32%.

@@             Coverage Diff              @@
##             master    #1295      +/-   ##
============================================
+ Coverage     81.97%   82.01%   +0.03%     
- Complexity     2527     2530       +3     
============================================
  Files           105      105              
  Lines          7169     7201      +32     
  Branches       2053     2058       +5     
============================================
+ Hits           5877     5906      +29     
  Misses          346      346              
- Partials        946      949       +3     
Flag Coverage Δ
unittests 82.01% <90.32%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t/ruleset/rules/chapter3/BooleanExpressionsRule.kt 90.60% <90.32%> (+<0.01%) ⬆️

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 7f116ac...ed49717. Read the comment docs.

Copy link
Member

@petertrr petertrr left a comment

Choose a reason for hiding this comment

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

LGTM


fun size(): Int = replacements.size

fun getTokenMapper(): TokenMapper<String> = orderTokenMapper
Copy link
Member

Choose a reason for hiding this comment

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

I think a getter for this is redundant, because you can have a public val with the same effect as private field with public getter

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for suggestion, fixed

### What's done:
* removed tokenMapper function and opened the variable orderedTokenMapper
@nulls nulls enabled auto-merge (squash) May 25, 2022 10:37
@nulls nulls merged commit 4656b80 into master May 25, 2022
@nulls nulls deleted the feature/boolean_expression_order branch May 25, 2022 10:52
@nulls nulls added this to the 1.1.1 milestone May 25, 2022
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.

BooleanExpressionsRule can generate incorrect code. Boolean conditions: support prefix negation
3 participants