Skip to content

Switch from BoolTrue to optional.Option[bool]#5693

Merged
6543 merged 15 commits into
woodpecker-ci:mainfrom
6543-forks:switch-from-booltrue-to-optional
Nov 4, 2025
Merged

Switch from BoolTrue to optional.Option[bool]#5693
6543 merged 15 commits into
woodpecker-ci:mainfrom
6543-forks:switch-from-booltrue-to-optional

Conversation

@6543

@6543 6543 commented Oct 26, 2025

Copy link
Copy Markdown
Member

as per #1835 (comment)

the BoolTrue is cursed as it exloites the bool type and use it inferted ... this mostly works but if you de-/serialize it things get messed up.

instead now we store if it was set or not and what it value was and make the defaulting to true if not set an explicit thing in our if logics ... this also alows us to reserialize it as much as we want

Comment thread pipeline/frontend/yaml/parse_test.go Outdated
@6543 6543 added bug Something isn't working refactor delete or replace old code bounty get some rewards if it got resolved and removed bounty get some rewards if it got resolved labels Oct 26, 2025
@6543 6543 requested a review from qwerty287 October 26, 2025 00:59
@6543 6543 removed the bug Something isn't working label Oct 26, 2025
@6543

6543 commented Oct 26, 2025

Copy link
Copy Markdown
Member Author

the difference is before we default to true by serializing now we default to true by in code bool operations

@codecov

codecov Bot commented Oct 26, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 15.38462% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.15%. Comparing base (393a598) to head (ca2d72d).

Files with missing lines Patch % Lines
pipeline/frontend/yaml/constraint/constraint.go 0.00% 6 Missing ⚠️
pipeline/frontend/yaml/constraint/path.go 28.57% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5693      +/-   ##
==========================================
- Coverage   21.20%   21.15%   -0.05%     
==========================================
  Files         425      424       -1     
  Lines       38367    38356      -11     
==========================================
- Hits         8135     8114      -21     
- Misses      29477    29488      +11     
+ Partials      755      754       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@qwerty287

Copy link
Copy Markdown
Contributor

I'm not that sure about this change. Yes, the BoolTrue logic is weird, but using it is quite easy as you just do b.Bool(). Now you have to do !b.Has() || b.Value(). This is quite error-prone I think, it's likely you forget something at some point, esp. the Has(). What would be better would be something like b.ValueWithDefault(true) so it returns true if it's not set.

@6543

6543 commented Oct 26, 2025

Copy link
Copy Markdown
Member Author

if it's about the lib being external we can just copy it into wp

@qwerty287

Copy link
Copy Markdown
Contributor

No, that's not my problem. I just don't like the syntax. If you would have a func ValueWithDefault(true) that returns true if the value isn't set that would be fine for me.

@6543

6543 commented Oct 26, 2025

Copy link
Copy Markdown
Member Author

No, that's not my problem. I just don't like the syntax. If you would have a func ValueWithDefault(true) that returns true if the value isn't set that would be fine for me.

hah nice we come up with the same idea ... was just going to suggest that to you :)

Comment thread pipeline/frontend/yaml/constraint/path.go Outdated
Comment thread pipeline/frontend/yaml/constraint/constraint.go Outdated
Comment thread pipeline/frontend/yaml/constraint/path.go Outdated
@6543

6543 commented Oct 27, 2025

Copy link
Copy Markdown
Member Author

done

Comment thread pipeline/frontend/yaml/constraint/constraint.go Outdated
@6543

6543 commented Oct 31, 2025

Copy link
Copy Markdown
Member Author

imported

@qwerty287

Copy link
Copy Markdown
Contributor

Linter fails but otherwise lgtm

@6543 6543 enabled auto-merge (squash) November 4, 2025 13:34
@6543 6543 merged commit dd0f593 into woodpecker-ci:main Nov 4, 2025
6 of 7 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request Nov 4, 2025
1 task
@6543 6543 deleted the switch-from-booltrue-to-optional branch November 4, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor delete or replace old code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants