8000 Replace IsSet(BoolFlag) with Bool(BoolFlag) by blukat29 · Pull Request #1910 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

Replace IsSet(BoolFlag) with Bool(BoolFlag) #1910

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Replace IsSet(BoolFlag) with Bool(BoolFlag) #1910

merged 1 commit into from
Jul 31, 2023

Conversation

blukat29
Copy link
Contributor
@blukat29 blukat29 commented Jul 28, 2023 •
edited
Loading

Proposed changes

Types of changes

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Further comments

I manually inspected every instance of ctx.IsSet() and the usage boils down to these cases:

  1. if IsSet( non-bool-flag ) { ... } kept untouched.
  2. if IsSet( bool-flag ) { cfg.BoolVar = true } is now
    if Bool( bool-flag ) { cfg.BoolVar = true }
    • I could have further simplified it into cfg.BoolVar = Bool( bool-flag ).
    • But I didn't for consistency with neighboring code lines such as in here.
  3. cfg.BoolVar = IsSet( bool-flag ) is now
    cfg.BoolVar = Bool( bool-flag )

@kjeom
Copy link
kjeom commented Jul 28, 2023

Are you finding these things more?

@blukat29
Copy link
Contributor Author

@kjeom See the updated PR description above.

@blukat29 blukat29 marked this pull request as ready for review July 31, 2023 05:40
@blukat29 blukat29 requested a review from ian0371 as a code owner July 31, 2023 05:40
@blukat29 blukat29 merged commit 5168145 into klaytn:dev Jul 31, 2023
@blukat29 blukat29 deleted the flags-isset-bool branch August 3, 2023 08:08
@aidan-kwon aidan-kwon mentioned this pull request Aug 3, 2023
Closed
20 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kjeom kjeom kjeom approved these changes

@aidan-kwon aidan-kwon aidan-kwon approved these changes

@ian0371 ian0371 Awaiting requested review from ian0371

Assignees

@blukat29 blukat29

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blukat29 @kjeom @aidan-kwon
0