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

Unnecessary If/Unless #89

Merged
merged 19 commits into from
Jul 7, 2024
Merged

Conversation

sodapopcan
Copy link
Contributor

Here's my first pass at collapsing what I'm calling "redundant booleans," ie:

if expr, do: true, else: false

I don't quite like that name and am still thinking on it but I needed something! It only takes case of do: true, else: false because I realized it could be taken further but I decided that isn't worth it. do: false, else: true could protentially be re-written with the if expression negated. You have to chose between ! and not (possibly making it configurable) and it seems like one of those things that should just be a credo warning.

I only dealt with leading comments on the if line though I can be more thorough. Again, it just didn't seem worth it since people usually write these "redundant booleans" pretty absent-mindedly and can't imagine they'd be full of comments in the do/else bodies. I'm also not exactly sure how to keep comments on the same line. "Leading comments" are comments both above and after a line. I'll study Sourceror a little more but I just wanted to get this up for review.

I'm certainly open to different ideas on the name! I was even thinking something as dumb as NoDoTrueElseFalse 😅 😅 😅

Thanks!

@NickNeck
Copy link
Member

Thanks a lot. I will take a look at the weekend.

@NickNeck
Copy link
Member

Already not looked at the code, but my suggestion for the name would be UnnecessaryIfUnless. As the name suggests, the implementation should also include unless.

To rewrite if expr, do: false, else: true to not expr would be nice, but we can leave this for later (or never).

@sodapopcan
Copy link
Contributor Author

sodapopcan commented Jun 29, 2024

All feedback has been addressed including rewriting the corresponding unlesses.

I'm happy to squash a succinct commit if/when you're happy with it!

README.md Show resolved Hide resolved
@sodapopcan sodapopcan force-pushed the redundant-booleans branch 2 times, most recently from f24eb4f to 38bcceb Compare June 29, 2024 13:27
@sodapopcan sodapopcan changed the title Redundant booleans Unnecessary If/Unless Jun 29, 2024
Copy link
Member

@NickNeck NickNeck left a comment

Choose a reason for hiding this comment

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

Very nice. Thank you.

@coveralls
Copy link

coveralls commented Jun 30, 2024

Pull Request Test Coverage Report for Build 298454e56b33de8ccb84c5e6d60c1c5841f3358b-PR-89

Details

  • 20 of 21 (95.24%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 91.5%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/recode/task/unnecessary_if_unless.ex 20 21 95.24%
Totals Coverage Status
Change from base Build a97ec42c4b51b478e237027cea09efa4e41f3539: 0.08%
Covered Lines: 958
Relevant Lines: 1047

💛 - Coveralls

@sodapopcan
Copy link
Contributor Author

Do you want me to squash this or does your CI handle that?

@NickNeck
Copy link
Member

NickNeck commented Jul 1, 2024

Do you want me to squash this or does your CI handle that?

I will handle this with GitHub.

@NickNeck NickNeck merged commit 8804b66 into hrzndhrn:main Jul 7, 2024
14 checks passed
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.

3 participants