Rust enums

[nimrod-starkware]

This change is 

[yuvalsw]

Great, thanks Nimrod!

@yuvalsw reviewed 1 of 3 files at r2.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @liorgold2 and @Stavbe)

---

blocks_typ/enums.typ line 18 at r2 (raw file):

#block_header(7, [Rust enums are great!])\
#what
\ Everyone knows what enums are — but knowing when to use them isn’t always obvious.

Rust enums are different than a general enum, so:

1. Don't say everyone knows what they are (python devs probably don't)
2. Refer to them as Rust enums, like in the title.

Code quote:

Everyone knows what enums are

---

blocks_typ/enums.typ line 26 at r2 (raw file):

struct LoggingConfig {
    enabled: bool,
    // If not enabled, we don't know the level.

This is not the problem here. Ehe real problem is mentioned later - you allow invalid configs (enabled with None for the level, or disabled with a level that doesn't mean anything).

Code quote:

// If not enabled, we don't know the level.

---

blocks_typ/enums.typ line 55 at r2 (raw file):

#tip[
  Planning to (de)serialize? Reach for a struct, not an enum.
Enums tend to confuse other languages.

WDYM? This is vague and confusing to me.

Code quote:

  Planning to (de)serialize? Reach for a struct, not an enum.
Enums tend to confuse other languages.

[Stavbe]

LGTM or wait a month ;)

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @liorgold2, @nimrod-starkware, and @yuvalsw)

---

blocks_typ/enums.typ line 55 at r2 (raw file):

Previously, yuvalsw wrote…

WDYM? This is vague and confusing to me.

I don't know, Lior said something about the serialization problem with enums. Anyway, changed to something I can relate to more

---

src/block_by_block.typ line 75 at r3 (raw file):

        #text(
            fill: black,
            size: 12pt,

It is an emergency

[Stavbe]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @liorgold2, @nimrod-starkware, and @yuvalsw)

[nimrod-starkware]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @liorgold2 and @yuvalsw)

---

blocks_typ/enums.typ line 55 at r2 (raw file):

Previously, Stavbe wrote…

I don't know, Lior said something about the serialization problem with enums. Anyway, changed to something I can relate to more

I think what Lior meant was this:
If for example you want to serialize from python to rust an object (i.e define class/struct with the same schema and use serde), then you can't use enum on the rust side as it defines different type of schema that won't be deserialized correctly by default.

[liorgold2]

@liorgold2 reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @nimrod-starkware and @yuvalsw)

---

blocks_typ/enums.typ line 18 at r3 (raw file):

#block_header(7, [Rust enums are great!])\
#what
\ Rust enums let you to name and limit the valid states a variable can take in your domain.

Remove.

Code quote:

 in your domain

---

blocks_typ/enums.typ line 19 at r3 (raw file):

#what
\ Rust enums let you to name and limit the valid states a variable can take in your domain.
\ If your type can only take on a few known values- you should enum it!\

Suggestion:

\ If your type can only take on a few known values - you should enum it!\

---

blocks_typ/enums.typ line 24 at r3 (raw file):

#how
#bad_code[```rust
struct LoggingConfig {

Actually, this example is not ideal, as the enum can be replaced with just Option<LogLevel> - no real need for the enum. I'll try to think of a better example.

Code quote:

struct LoggingConfig {

---

blocks_typ/enums.typ line 37 at r3 (raw file):

}
```]
\ This struct enables invalid configurations like LoggingConfig { enabled: true, level: None } or { enabled: false, level: Some}.

Suggestion:

\ This struct enables invalid configurations like `LoggingConfig { enabled: true, level: None }`.

---

blocks_typ/enums.typ line 50 at r3 (raw file):

        }
    }
}

Suggestion:

enum LoggingConfig {
    Disabled,
    Enabled(LogLevel),
}
impl LoggingConfig {
    fn log(&self, message: &str) {
        if let Self::Enabled(level) = self { // No risk of panicking.
            log(level, message);
        }
    }
}

---

blocks_typ/enums.typ line 50 at r3 (raw file):

        }
    }
}

I'd consider removing the less-interesting part to save some space and clutter (Same below)

Suggestion:

#good_code[```rust
enum LoggingConfig {
    Disabled,
    Enabled(LogLevel),
}
...
if let Self::Enabled(level) = self { // No risk of panicking.
    log(level, message);
}

---

blocks_typ/enums.typ line 52 at r3 (raw file):

}
```]
#call_for_action[ Use enums rather then strings in the config, and you’l never has to spel corectly agan.

I'm assuming the spelling mistakes are here on purpose, but I'm not convinced it's a good joke :)

Code quote:

#call_for_action[ Use enums rather then strings in the config, and you’l never has to spel corectly agan.

[liorgold2]

Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @nimrod-starkware and @yuvalsw)

---

blocks_typ/enums.typ line 24 at r3 (raw file):

Previously, liorgold2 wrote…

Actually, this example is not ideal, as the enum can be replaced with just Option<LogLevel> - no real need for the enum. I'll try to think of a better example.

After discussing with @yuvalsw, we came up with:

struct InputSource {
    is_stdin: bool, // Either a file or stdin.
    path: Option<String>,
}

vs

enum InputSource {
    Stdin,
    File(String),
}

[yuvalsw]

@yuvalsw reviewed 1 of 4 files at r3.
Reviewable status: 1 of 4 files reviewed, 12 unresolved discussions (waiting on @nimrod-starkware and @Stavbe)

---

blocks_typ/enums.typ line 18 at r3 (raw file):

#block_header(7, [Rust enums are great!])\
#what
\ Rust enums let you to name and limit the valid states a variable can take in your domain.

Suggestion:

et you name and limit the

---

blocks_typ/enums.typ line 37 at r3 (raw file):

}
```]
\ This struct enables invalid configurations like LoggingConfig { enabled: true, level: None } or { enabled: false, level: Some}.

the snippets should be quoted as code snippets.
Also - see suggestion

Suggestion:

{ enabled: true, level: None } or { enabled: false, level: Some(...) }.

---

blocks_typ/enums.typ line 52 at r3 (raw file):

}
```]
#call_for_action[ Use enums rather then strings in the config, and you’l never has to spel corectly agan.

not sure if this is part of the pun, so if not, should be "than"

Code quote:

ums rather then str

[nimrod-starkware]

Reviewable status: 1 of 4 files reviewed, 12 unresolved discussions (waiting on @liorgold2 and @Stavbe)

---

blocks_typ/enums.typ line 24 at r3 (raw file):

Previously, liorgold2 wrote…

After discussing with @yuvalsw, we came up with:

struct InputSource {
    is_stdin: bool, // Either a file or stdin.
    path: Option<String>,
}

vs

enum InputSource {
    Stdin,
    File(String),
}

Why is it different?
Can't it be replaced with Option<String>?

I think maybe the example is clearer if we have this:

struct LoggingConfig {
   enabled: bool,
   level: LogLevel
}

and it's unclear what should be the value of level when logging is disabled.

[yuvalsw]

Reviewable status: 1 of 4 files reviewed, 12 unresolved discussions (waiting on @liorgold2, @nimrod-starkware, and @Stavbe)

---

blocks_typ/enums.typ line 24 at r3 (raw file):

Previously, nimrod-starkware wrote…

Why is it different?
Can't it be replaced with Option<String>?

I think maybe the example is clearer if we have this:

struct LoggingConfig {
   enabled: bool,
   level: LogLevel
}

and it's unclear what should be the value of level when logging is disabled.

you can change in Lior's bad example to String rather than Option<String> if you want to achieve this confusion