[ShahakShama]

Note

Low Risk
Low risk documentation-only change that adjusts style guidance but does not affect runtime behavior.

Overview
Updates style.md by replacing the Boolean Function Parameters guidance (and its Option-parameter discussion) with a broader Unnamed Function Parameters section.

The new guidance emphasizes designing APIs that make argument meaning explicit, recommending enums or argument structs (and, as a fallback, named variables at the call site) to avoid unclear calls like foo(true, false) or bar(None).

^{Written by Cursor Bugbot for commit af9a292. This will update automatically on new commits. Configure here.}

[reviewable-StarkWare]

This change is 

[Stavbe]

@Stavbe made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ShahakShama).

---

style.md line 226 at r1 (raw file):

// This will compile because without dry run we can have execution status enabled
TransactionBuilder::new().without_dry_run().with_execution_status_enabled();
// This will NOT compile. The struct returned from without_dry_run does not implement with_execution_status_disabled

what about
TransactionBuilder::new().with_execution_status_enabled();.without_dry_run()?
it will compile.
I would delete the " an advantage of a builder part" and maybe publish a bbb on that

[ShahakShama]

@ShahakShama made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Stavbe).

---

style.md line 226 at r1 (raw file):

Previously, Stavbe wrote…

what about
TransactionBuilder::new().with_execution_status_enabled();.without_dry_run()?
it will compile.
I would delete the " an advantage of a builder part" and maybe publish a bbb on that

It won't compile if you make it not compile (the same way you make the example not compile)

[Stavbe]

@Stavbe made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @liorgold2 and @yuvalsw).

---

style.md line 226 at r1 (raw file):

Previously, ShahakShama wrote…

It won't compile if you make it not compile (the same way you make the example not compile)

I don't like this option I preffer to delete it. @liorgold2 @yuvalsw WDYT?

[ShahakShama]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• add comment in return value section to split checking the value and performing action to 2 lines #13144 : 2 dependent PRs (#13145 , #13146 )
• remove bad example in foo_or_else section #13141
• improve the starting paragraphs of testing section #13139
• improve macros section #13137
• remove unnecessary misc sections #13131
• reorder sections (move AI and Testing up) #13118
• remove open issues section #13115
• improve external dependencies section #13114
• remove github smell testing section #13113
• create misc section, move stuff into it, and order by importance (no erasure of anything yet) #13112
• change file layout section #13078
• add section on sleep in tests #13073
• add section on dependency injection #13072
• move expect message to a separate section with more explanation #13071
• add directory layout section #12950
• rename allocations and add &Option<T> #12781
• remove laziness  #11591
• rewrite types section #11588
• more comments  #11587
• more comments  #11586
• more comments  #11585 👈 (View in Graphite)
• more comments  #11584
• arithmetic: remove ditto for other mathematical operations. #11848
• gilad/style-guide

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Stavbe]

Since Lior and Yuval didn't respond, I asked them f2f and those were the comments:

@Stavbe made 6 comments.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on liorgold2, ShahakShama, and yuvalsw).

---

style.md line 202 at r1 (raw file):

impl Logger for NoLogging {/* no-op implementation */}
connect("server", NoLogging)

remove

Code quote:

3. Define zero sided types that implement a trait and have the parameter be generic
```rust
struct NoLogging;
impl Logger for NoLogging {/* no-op implementation */}
connect("server", NoLogging)

___
*[`style.md` line 205 at r1](https://reviewable.io/reviews/starkware-libs/sequencer/11585#-OmA6BYeAoh8ecx7H2Zc:-OmA6BYeAoh8ecx7H2Zd:bdwbkqd) ([raw file](https://github.com/starkware-libs/sequencer/blob/2bd524258fc171794a41865da82d714fef3a0065/style.md#L205)):*
> ```Markdown
> 4. Make a builder API where each parameter is a function that adds the parameter to the builder
> ```rust
> TransactionBuilder::new().with_dry_run(true).with_execution_status(false);
> ```

.with_execution_status(false).build();



_Code quote:_
```Markdown
.with_execution_status(false);

---

style.md line 208 at r1 (raw file):

If you can't control the API, add names at the callsite by defining variables

Remove.
This is simple and good in more senarios rather then only when you can't control the API.

Code quote:

If you can't control the API,

---

style.md line 213 at r1 (raw file):

let is_dry_run = true;
let is_executable = false;
create_transaction(is_dry_run, is_executable)

This should be 4 now.

Code quote:

```rust
let is_dry_run = true;
let is_executable = false;
create_transaction(is_dry_run, is_executable)

---

style.md line 228 at r1 (raw file):

// This will NOT compile. The struct returned from without_dry_run does not implement with_execution_status_disabled
TransactionBuilder::new().without_dry_run().with_execution_status_disabled();

please remove this is for block by block and not for style guide.

Code quote:

**Rationale**: Callsite is more readable, without having to resort to using auxiliary args.

**Rationale**: An advantage of a builder (option 4) over the rest of the options is that it allows
enforcing a set of constraints at the compiler level by having different types of TransactionBuilder that have implemented different methods.
```rust
// These will all compile because in dry run both execution statuses are valid
TransactionBuilder::new().with_dry_run().with_execution_status_enabled();
TransactionBuilder::new().with_dry_run().with_execution_status_disabled();
// This will compile because without dry run we can have execution status enabled
TransactionBuilder::new().without_dry_run().with_execution_status_enabled();
// This will NOT compile. The struct returned from without_dry_run does not implement with_execution_status_disabled
TransactionBuilder::new().without_dry_run().with_execution_status_disabled();

<!-- Sent from Reviewable.io -->

[ShahakShama]

@ShahakShama made 6 comments.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on liorgold2, Stavbe, and yuvalsw).

---

style.md line 202 at r1 (raw file):

Previously, Stavbe wrote…

remove

Done.

---

style.md line 205 at r1 (raw file):

Previously, Stavbe wrote…

.with_execution_status(false).build();

Done.

---

style.md line 208 at r1 (raw file):

Previously, Stavbe wrote…

Remove.
This is simple and good in more senarios rather then only when you can't control the API.

I disagree. This should be a last resort IMO

---

style.md line 213 at r1 (raw file):

Previously, Stavbe wrote…

This should be 4 now.

Done.

---

style.md line 226 at r1 (raw file):

Previously, Stavbe wrote…

I don't like this option I preffer to delete it. @liorgold2 @yuvalsw WDYT?

Per your next comment I erased this section

---

style.md line 228 at r1 (raw file):

Previously, Stavbe wrote…

please remove this is for block by block and not for style guide.

Done.

[ShahakShama]

@ShahakShama made 1 comment.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on liorgold2, Stavbe, and yuvalsw).

---

style.md line 201 at r2 (raw file):

TransactionBuilder::new().with_dry_run(true).with_execution_status(false).build();

4. If there's a single parameter, you can rename the function to capture the name of the parameter too

I added a new one. WDYT?

[Stavbe]

@Stavbe reviewed 1 file and all commit messages, and resolved 6 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion.

[github-actions]

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!