StudioInfinity/dyna3
2
0
Fork 2
Code Issues 57 Pull requests 1 Projects 3 Releases 4 Wiki Activity Actions

Add trailing commas and clean up formatting #108

Merged
glen merged 5 commits from Vectornaut/dyna3:trailing-commas into main 2025-08-04 23:34:35 +00:00
Conversation 3 Commits 5 Files changed 12 +310 -297
Vectornaut commented 2025-08-01 21:30:49 +00:00
Member
Copy link

Trailing commas pull request

Goals

  • Primary: Switch to using trailing commas, addressing issue #99.
  • Secondary: Clean up formatting annoyances and inconsistencies found in the process of adding trailing commas.

The changes in this pull request are meant to be purely cosmetic: I doubt they'll affect the abstract syntax tree. I've also been jotting down simplifications that are more likely to affect the abstract syntax tree, with the intention of applying them in a future cleanup PR.

Changes

The changes in this pull request are made to enforce the following style rules. The trailing commas rule is a change from my previous style. The other rules are things I'd already been trying to do.

Trailing commas (new)

Use trailing commas whenever a comma-separated list of more than one item is split over multiple lines.

  • We leave out the trailing comma for single-argument function calls where we put the argument is on its own line. We use a trailing comma for all other single-item lists where the item is on its own line.
  • Commas are never required after match arms wrapped in blocks, so we leave them out, as recommended but we've decided to always use them, as discussed here.

Structure expressions

In structure expressions:

  • Use the field init shorthand when possible.
  • Put a space between the structure name and the opening brace.

Function declarations

When a function declaration is very long (more than 100 columns or so), split its parameters over multiple lines.

Imports

Alphabetize imports, subordinate to the following ordering:

  1. Constants and static variables
  2. Functions
  3. Types
  4. Modules

As an exception, put the Sycamore prelude module before other modules, instead of in alphabetical order.

Method

I used Rustfmt in check mode to help find places where trailing commas should be added:

cargo +nightly fmt --check

⚠️ Make sure to use the --check flag to keep Rustfmt from reformatting our code!

The Rustfmt output is very long, because my current style differs from the Rustfmt standard in many ways. You can reduce the output size by configuring Rustfmt with the following options, placed in app-proto/rustfmt.toml.

array_width = 80
chain_width = 80
imports_layout = "HorizontalVertical"
match_block_trailing_comma = true
overflow_delimited_expr = true
struct_lit_width = 40
struct_variant_width = 60
use_field_init_shorthand = true

I haven't checked this configuration file in because I don't expect to use Rustfmt regularly. There are some style choices I've been using which Rustfmt can't be configured to accept and which would be really annoying for me to give up. This adds a lot of noise to the Rustfmt output in check mode, and prevents me from using Rustfmt as intended to auto-format our code.

# Trailing commas pull request ## Goals - **Primary:** Switch to using trailing commas, addressing issue #99. - **Secondary:** Clean up formatting annoyances and inconsistencies found in the process of adding trailing commas. The changes in this pull request are meant to be purely cosmetic: I doubt they'll affect the abstract syntax tree. I've also been jotting down simplifications that are more likely to affect the abstract syntax tree, with the intention of applying them in a future cleanup PR. ## Changes The changes in this pull request are made to enforce the following style rules. The trailing commas rule is a change from my previous style. The other rules are things I'd already been trying to do. ### Trailing commas (new) Use trailing commas whenever a comma-separated list of more than one item is split over multiple lines. - We leave out the trailing comma for single-argument function calls where we put the argument is on its own line. We use a trailing comma for all other single-item lists where the item is on its own line. - Commas are never required after match arms wrapped in blocks, ~~so we leave them out, as [recommended](https://doc.rust-lang.org/style-guide/expressions.html#match)~~ but we've decided to always use them, as discussed [here](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3140). ### Structure expressions In structure expressions: - Use the [field init shorthand](https://doc.rust-lang.org/book/ch05-01-defining-structs.html#using-the-field-init-shorthand) when possible. - Put a space between the structure name and the opening brace. ### Function declarations When a function declaration is very long (more than 100 columns or so), split its parameters over multiple lines. ### Imports Alphabetize imports, subordinate to the following ordering: 1. Constants and static variables 2. Functions 3. Types 4. Modules As an exception, put the Sycamore [prelude](https://docs.rs/sycamore/latest/sycamore/prelude/index.html) module before other modules, instead of in alphabetical order. ## Method I used [Rustfmt](https://rust-lang.github.io/rustfmt/) in check mode to help find places where trailing commas should be added: ```bash cargo +nightly fmt --check ``` ⚠️ Make sure to use the `--check` flag to keep Rustfmt from reformatting our code! The Rustfmt output is very long, because my current style differs from the Rustfmt standard in many ways. You can reduce the output size by configuring Rustfmt with the following options, placed in `app-proto/rustfmt.toml`. ```toml array_width = 80 chain_width = 80 imports_layout = "HorizontalVertical" match_block_trailing_comma = true overflow_delimited_expr = true struct_lit_width = 40 struct_variant_width = 60 use_field_init_shorthand = true ``` I haven't checked this configuration file in because I don't expect to use Rustfmt regularly. There are some style choices I've been using which Rustfmt can't be configured to accept and which would be really annoying for me to give up. This adds a lot of noise to the Rustfmt output in check mode, and prevents me from using Rustfmt as intended to auto-format our code.
Vectornaut added 1 commit 2025-08-01 21:30:49 +00:00
Add trailing commas; clean up formatting
All checks were successful
/ test (pull_request) Successful in 3m55s
Details
af59166906
glen reviewed 2025-08-01 22:39:58 +00:00
glen left a comment
Owner
Copy link

May look like a lot, but mostly it's reflections on useful layout and some musings on what we may want in Husht ultimately, with just a few specific requests for changes. Anyhow, it seems as though as long as we are bothering with some formatting stuff, we may as well end up as comfortable as possible with the results, and have them seem as sustainable as possible.

May look like a lot, but mostly it's reflections on useful layout and some musings on what we may want in Husht ultimately, with just a few specific requests for changes. Anyhow, it seems as though as long as we are bothering with some formatting stuff, we may as well end up as comfortable as possible with the results, and have them seem as sustainable as possible.
app-proto/src/components/diagnostics.rs Outdated
@ -16,4 +16,4 @@
impl DiagnosticsState {
fn new(initial_tab: String) -> DiagnosticsState {
DiagnosticsState {
glen commented 2025-08-01 21:45:51 +00:00
Owner
Copy link

As a mostly uninformed question about Rust syntax, why is/must DiagnosticsState mentioned four times in quick succession here? The first is to declare a new data type, so is of course unavoidable. I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common? Is the third a type annotation and the fourth a constructor? If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of new? (If I recall, new has no special status in Rust, so we can't hope Rust will just know its return type should be the struct being impled. But the type is patent from the only exit from the function body...)

It would be great to reduce this redundancy; of course we may ultimately just have to introduce some mechanisms for it in Husht, if there's little to be done within Rust...

As a mostly uninformed question about Rust syntax, why is/must DiagnosticsState mentioned _four_ times in quick succession here? The first is to declare a new data type, so is of course unavoidable. I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common? Is the third a type annotation and the fourth a constructor? If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of `new`? (If I recall, `new` has no special status in Rust, so we can't hope Rust will just know its return type should be the struct being `impl`ed. But the type is patent from the only exit from the function body...) It would be great to reduce this redundancy; of course we may ultimately just have to introduce some mechanisms for it in Husht, if there's little to be done within Rust...
Vectornaut commented 2025-08-02 09:23:08 +00:00
Author
Member
Copy link

I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common?

I don't know of any more compact syntax. If there were one, it would probably be mentioned here. I think the current syntax is quite usable and readable. I also think there's an advantage to having only one syntax for an implementation block: you can find all of the implementation blocks for a non-parametric structure Sandwich by skimming or searching for blocks of the forms shown here:

impl Sandwich
impl Sandwich</* concrete type */>
impl</* generic type: perhaps with bounds */> Sandwich</* generic type */>

Is the third a type annotation and the fourth a constructor?

Yes.

If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of new?

I don't think so. The thing that comes before the braces in a struct expression is something called a path in expression, which looks like it's supposed to be a specific type specifier like Sandwich or snack::Sandwich or super::snack::Sandwich or Sandwich<Cucumber>.

The code below shows that Rust can infer type parameters in a struct expression, but I don't think there's syntax to request inference for the whole type.

struct NamedValue<T> {
    name: String,
    value: T,
}

fn the_answer() -> NamedValue<i32> {
    NamedValue {
        name: "The answer".to_string(),
        value: 42
    }
}

fn main() {
    let NamedValue { name, value } = the_answer();
    print!("{name} is {value}");
}

I guess it could be reasonable to use _ as a placeholder for inferred types:

let _ { name, value } = the_answer();

After all, it can already be used as a placeholder for inferred type parameters in expressions like this:

let list: Vec<_> = [4, 6, 8, 12, 20]
    .into_iter()
    .map(|n| format!("{n}-hedron"))
    .collect();

(In this example, the compiler can't infer what type of collection we want, because collect<B> is a generic function that can return collections of various types. Once we tell the compiler that we want a Vec, though, we can ask it to infer the type parameter that specifies the element type.)

> I think the second is to tie some methods to that datatype; is there no more compact syntax for doing so when you immediately want to tie some methods, as is very common? I don't know of any more compact syntax. If there were one, it would probably be mentioned [here](https://doc.rust-lang.org/stable/book/ch05-03-method-syntax.html). I think the current syntax is quite usable and readable. I also think there's an advantage to having only one syntax for an implementation block: you can find all of the implementation blocks for a non-parametric structure `Sandwich` by skimming or searching for blocks of the forms shown [here](https://doc.rust-lang.org/rust-by-example/generics/impl.html): ```rust impl Sandwich impl Sandwich</* concrete type */> impl</* generic type: perhaps with bounds */> Sandwich</* generic type */> ``` > Is the third a type annotation and the fourth a constructor? Yes. > If so, is there no way for Rust to simply infer the return type from the explicit type of the return value in the body of new? I don't think so. The thing that comes before the braces in a [struct expression](https://doc.rust-lang.org/reference/expressions/struct-expr.html) is something called a [*path in expression*](https://doc.rust-lang.org/reference/paths.html#paths-in-expressions), which looks like it's supposed to be a specific type specifier like `Sandwich` or `snack::Sandwich` or `super::snack::Sandwich` or `Sandwich<Cucumber>`. The code below shows that Rust can infer type parameters in a struct expression, but I don't think there's syntax to request inference for the whole type. ```rust struct NamedValue<T> { name: String, value: T, } fn the_answer() -> NamedValue<i32> { NamedValue { name: "The answer".to_string(), value: 42 } } fn main() { let NamedValue { name, value } = the_answer(); print!("{name} is {value}"); } ``` I guess it could be reasonable to use `_` as a placeholder for inferred types: ```rust let _ { name, value } = the_answer(); ``` After all, it can already be used as a placeholder for inferred type parameters in expressions like this: ```rust let list: Vec<_> = [4, 6, 8, 12, 20] .into_iter() .map(|n| format!("{n}-hedron")) .collect(); ``` (In this example, the compiler can't infer what type of collection we want, because [`collect<B>`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.collect) is a generic function that can return collections of various types. Once we tell the compiler that we want a `Vec`, though, we can ask it to infer the type parameter that specifies the element type.)
glen commented 2025-08-02 16:39:48 +00:00
Owner
Copy link

I guess it could be reasonable to use _ as a placeholder for inferred types:

let _ { name, value } = the_answer();

This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say?

On the page you link, it does mention that in an impl block, Self is an abbreviation for the type being impled. So that suggests we could write

struct DiagnosticsState {
    active_tab: Signal<String>,
}

impl DiagnosticsState {
    fn new(initial_tab: String) -> Self {
        Self { active_tab: create_signal(initial_tab) }
    }
}

What do you think? I'm just struggling with the virtue of design decisions on the part of Rust that would lead to the type name being written out four time in such a short bit of code. I don't see anything misleading or ambiguous about syntax like:

class DiagnosticsState  // transform to a struct declaration
   active_tab: Signal<String>

   fn new(initial_tab: String)  // fns in a class automatically get put in a impl block; return type inferred
      DiagnosticsState { active_tab: create_signal(initial_tab) } // or Self { ... }

as a possible Husht rendering of this. Anyhow, seems like the only action item to consider here is whether to use Self as an abbreviation, presuming it works fine. Seems pretty sensible for a "new" function, at least.

> I guess it could be reasonable to use _ as a placeholder for inferred types: > `let _ { name, value } = the_answer();` This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say? On the page you link, it does mention that in an `impl` block, `Self` is an abbreviation for the type being impled. So that suggests we could write ``` struct DiagnosticsState { active_tab: Signal<String>, } impl DiagnosticsState { fn new(initial_tab: String) -> Self { Self { active_tab: create_signal(initial_tab) } } } ``` What do you think? I'm just struggling with the virtue of design decisions on the part of Rust that would lead to the type name being written out four time in such a short bit of code. I don't see anything misleading or ambiguous about syntax like: ``` class DiagnosticsState // transform to a struct declaration active_tab: Signal<String> fn new(initial_tab: String) // fns in a class automatically get put in a impl block; return type inferred DiagnosticsState { active_tab: create_signal(initial_tab) } // or Self { ... } ``` as a possible Husht rendering of this. Anyhow, seems like the only action item to consider here is whether to use Self as an abbreviation, presuming it works fine. Seems pretty sensible for a "new" function, at least.
Vectornaut commented 2025-08-04 03:04:28 +00:00
Author
Member
Copy link

On the page you link, it does mention that in an impl block, Self is an abbreviation for the type being impled. So that suggests we could write […]. What do you think?

Oh, yes, we could do that. I mostly see Self used in implementations of traits, like Clone and From, but nothing would stop us from using it in the basic implementation of a structure. This style seems to be used to some extent in the source code for the Rust standard library and for nalgebra. For example:

  • It's used for the return type, but not the returned structure, in Vec::new.
  • It's used for the return type, but not the returned structure, in ColPivQR::new from nalgebra.
  • It's not used at all in BTreeSet::new_in—which is newly added code, and therefore might reflect current conventions.

The main downside I see to this style is that it can make the source harder to navigate, because it's context-dependent.

If we use this style, I think we should use it consistently for every structure. Let me know if you'd like me to proceed with reformatting all our structure implementations in this way.

I guess it could be reasonable to use _ as a placeholder for inferred types:

This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say?

Correct.

> On the page you link, it does mention that in an impl block, Self is an abbreviation for the type being impled. So that suggests we could write […]. What do you think? Oh, yes, we could do that. I mostly see `Self` used in implementations of traits, like `Clone` and `From`, but nothing would stop us from using it in the basic implementation of a structure. This style seems to be used to some extent in the source code for the Rust standard library and for nalgebra. For example: - It's used for the return type, but not the returned structure, in [`Vec::new`](https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#434). - It's used for the return type, but not the returned structure, in [`ColPivQR::new`](https://docs.rs/nalgebra/latest/src/nalgebra/linalg/col_piv_qr.rs.html#59-92`) from nalgebra. - It's not used at all in [`BTreeSet::new_in`](https://doc.rust-lang.org/src/alloc/collections/btree/set.rs.html#364)—which is newly added code, and therefore might reflect current conventions. The main downside I see to this style is that it can make the source harder to navigate, because it's context-dependent. If we use this style, I think we should use it consistently for every structure. Let me know if you'd like me to proceed with reformatting all our structure implementations in this way. >> I guess it could be reasonable to use _ as a placeholder for inferred types: > > This isn't actually allowed syntax, correct? Just something one might plausibly implement in Husht, say? Correct.
glen commented 2025-08-04 09:03:33 +00:00
Owner
Copy link

Well, I like the reduced rendundancy of Self, especially for long struct names. So if you're comfortable with it, yes please let's use that abbreviation, either in this PR or the next as seems better to you.

Well, I like the reduced rendundancy of Self, especially for long struct names. So if you're comfortable with it, yes please let's use that abbreviation, either in this PR or the next as seems better to you.
Vectornaut commented 2025-08-04 17:27:11 +00:00
Author
Member
Copy link

Let's do it in the next PR, along with renaming the test assembly loader functions. I think those two mass name changes are more similar to each other than to the changes this PR was intended for.

Let's do it in the next PR, along with renaming the test assembly loader functions. I think those two mass name changes are more similar to each other than to the changes this PR was intended for.
glen commented 2025-08-04 21:25:09 +00:00
Owner
Copy link

OK, again, please put it in whatever new issue you made/are making for the function name changes. Resolving, and will merge this PR as soon as I have a moment to run-test it.

OK, again, please put it in whatever new issue you made/are making for the function name changes. Resolving, and will merge this PR as soon as I have a moment to run-test it.
Vectornaut commented 2025-08-04 21:55:06 +00:00
Author
Member
Copy link

This is now issue #110. (To help with organization, I opened a separate issue instead of mashing it together with #109).

This is now issue #110. (To help with organization, I opened a separate issue instead of mashing it together with #109).
glen marked this conversation as resolved
app-proto/src/components/diagnostics.rs
@ -19,3 +19,3 @@
DiagnosticsState {
active_tab: create_signal(initial_tab)
active_tab: create_signal(initial_tab),
}
glen commented 2025-08-01 21:48:24 +00:00
Owner
Copy link

As a mild convention, should we prefer things like

DiagnosticsState {active_tab: create_signal(initial_tab)}

when they easily fit on one line? As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium...

p.s. I see now that in fact you have re-single-lined several such constructors in other files, so it seems to me this one should be as well, right?

As a mild convention, should we prefer things like ``` DiagnosticsState {active_tab: create_signal(initial_tab)} ``` when they easily fit on one line? As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium... p.s. I see now that in fact you have re-single-lined several such constructors in other files, so it seems to me this one should be as well, right?
Vectornaut commented 2025-08-02 05:42:17 +00:00
Author
Member
Copy link

As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium...

Oh, yeah, I remember you mentioning that. I've collapsed the structure expression onto one line in commit ebad512. I had put active_tab: /* ... */ on its own line to make the create_signal call more readable, but it's fine if that's not worth the cost in vertical space.

> As you may recall, I am averse to typographical conventions that burn vertical lines of code with no content, because I find allowing more code semantics to fit on the screen at once to be an aid to code comprehension, and it is vertical real estate that is at a premium... Oh, yeah, I remember you mentioning that. I've collapsed the structure expression onto one line in commit ebad512. I had put `active_tab: /* ... */` on its own line to make the `create_signal` call more readable, but it's fine if that's not worth the cost in vertical space.
glen marked this conversation as resolved
app-proto/src/components/diagnostics.rs
@ -35,3 +35,3 @@
Ok(_) => "",
Err(_) => "invalid"
Err(_) => "invalid",
}
glen commented 2025-08-01 21:53:00 +00:00
Owner
Copy link

Confused; I thought match branches didn't need commas... seems inconsistent with the vicinity of Err(Message) => in assembly.rs above.

If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces, then I would mildly urge keeping commas after every match branch for consistency. I don't see why match branches should have different punctuation depending on whether or not they happen to be expressions or whatever brace-enclosed things are. Perhaps we shall have to make the commas unnecessary in both contexts in Husht...

p.s. I contemplate another option below which is to always use the braces, and no commas, which might be fine as well until Husht can uniformize this...

Confused; I thought match branches didn't need commas... seems inconsistent with the vicinity of `Err(Message) =>` in assembly.rs above. If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces, then I would mildly urge keeping commas after every match branch for consistency. I don't see why match branches should have different punctuation depending on whether or not they happen to be expressions or whatever brace-enclosed things are. Perhaps we shall have to make the commas unnecessary in both contexts in Husht... p.s. I contemplate another option below which is to always use the braces, and no commas, which might be fine as well until Husht can uniformize this...
Vectornaut commented 2025-08-02 06:04:53 +00:00
Author
Member
Copy link

If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces […]

Yes, that's the point.

[…] then I would mildly urge keeping commas after every match branch for consistency.

Done, in commit bfd5d8e. I've updated the pull request description to reflect this. Rustfmt can be configured to use this style by setting match_block_trailing_comma to true.

> If the point is that a comma is required when the branch is an expression without braces and optional when the branch is enclosed in braces […] Yes, that's the point. > […] then I would mildly urge keeping commas after every match branch for consistency. Done, in commit bfd5d8e. I've updated the pull request description to reflect this. Rustfmt can be configured to use this style by setting [`match_block_trailing_comma`](https://rust-lang.github.io/rustfmt/?version=v1.8.0&search=#match_block_trailing_comma) to `true`.
glen marked this conversation as resolved
app-proto/src/components/display.rs Outdated
@ -57,0 +59,4 @@
color: ElementColor,
opacity: f32,
highlight: f32,
) {
glen commented 2025-08-01 21:57:28 +00:00
Owner
Copy link

These options will (a) clearly be subject to change and expansion as dyna3 matures, and (b) soon be too numerous to reiterate on every concrete function that needs such options, and so will have to be folded into some flexible options data structure that can specify none, some, or all of the options and fall back on defaults for the unspecified ones. But perhaps this PR is not the time to do it? I just mention it because with respecting the 80-char line limit (which I heartily approve), the multiple signatures with this same list of arguments is now burning a lot of vertical space, slightly non-DRYly.

p.s. but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line, i.e. I don't see a particular reason to be wedded to one parameter/argument per line just because a call is broken onto more than one line, and indeed in other places in the code we do bunch some arguments on a single line in multi-line calls.

These options will (a) clearly be subject to change and expansion as dyna3 matures, and (b) soon be too numerous to reiterate on every concrete function that needs such options, and so will have to be folded into some flexible options data structure that can specify none, some, or all of the options and fall back on defaults for the unspecified ones. But perhaps this PR is not the time to do it? I just mention it because with respecting the 80-char line limit (which I heartily approve), the multiple signatures with this same list of arguments is now burning a lot of vertical space, slightly non-DRYly. p.s. but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line, i.e. I don't see a particular reason to be wedded to one parameter/argument per line just because a call is broken onto more than one line, and indeed in other places in the code we do bunch some arguments on a single line in multi-line calls.
Vectornaut commented 2025-08-02 06:20:31 +00:00
Author
Member
Copy link

Yes, I think that revising the SceneSpheres and ScenePoints structures at some point would be a good idea.

But perhaps this PR is not the time to do it?

I agree. Like I said in the description, I'd like to limit this PR to cosmetic changes which are unlikely to affect the abstract syntax tree.

Yes, I think that revising the `SceneSpheres` and `ScenePoints` structures at some point would be a good idea. > But perhaps this PR is not the time to do it? I agree. Like I said in the description, I'd like to limit this PR to cosmetic changes which are unlikely to affect the abstract syntax tree.
Vectornaut commented 2025-08-02 06:28:01 +00:00
Author
Member
Copy link

but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line

Sure. Do either of the following groupings look good to you?

fn push(
    &mut self, representation: DVector<f64>,
    color: ElementColor, opacity: f32, highlight: f32, selected: bool,
)

fn push(
    &mut self,
    representation: DVector<f64>,
    color: ElementColor, opacity: f32, highlight: f32, selected: bool,
)

Note that I'm using the parameter list for the analogous ScenePoints::push function, which includes the selected parameter.

> but also see a later comment that another way to go at the moment might be to go ahead and group some of these option arguments onto a single line Sure. Do either of the following groupings look good to you? ```rust fn push( &mut self, representation: DVector<f64>, color: ElementColor, opacity: f32, highlight: f32, selected: bool, ) ``` ```rust fn push( &mut self, representation: DVector<f64>, color: ElementColor, opacity: f32, highlight: f32, selected: bool, ) ``` Note that I'm using the parameter list for the analogous `ScenePoints::push` function, which includes the `selected` parameter.
glen commented 2025-08-02 23:46:53 +00:00
Owner
Copy link

Either of your two suggestions is fine; I agree on the sensibility of clustering the "appearance options" parameters/arguments, as those are the ones that will likely be collected into some sort of Style object in the future.

Either of your two suggestions is fine; I agree on the sensibility of clustering the "appearance options" parameters/arguments, as those are the ones that will likely be collected into some sort of Style object in the future.
Vectornaut commented 2025-08-04 03:11:51 +00:00
Author
Member
Copy link

Done in commit e67a658.

Done in commit e67a658.
glen marked this conversation as resolved
app-proto/src/components/display.rs Outdated
@ -896,3 +924,3 @@
};
}
},
)
glen commented 2025-08-01 22:11:56 +00:00
Owner
Copy link

this function that just got comma-terminated is the value of a keyword argument onclick:move=fn ... (unchanged in this PR). While we are doing formatting, is it legal in keyword arguments to have whitespace: onclick:move = fn ... ? If so, please also adopt such a convention, as it is far more readable.

If not, I think the syntax design here really exposes the hypocrisy of the sanctimonious "oh whitespace-sensitive languages are so terrible and dangerous etc." because here we have a whitespace sensitivity that makes the programs less easily visually processed (and violate usual rules of punctuation, which include whitespace around =).

I assume the state of affairs is the latter, maybe because a keyword argument needs to be distinguished from an in-line assignment, but using the lack of whitespace to do so when (I think) in other contexts an assignment could elide the whitespace (or if not, why can you write a+1 but not a=1)? But these fine points of whitespace-based semantics, at the cost of readability, seem nutty to me. Hopefully we can do better in Husht...

this function that just got comma-terminated is the value of a keyword argument `onclick:move=fn ...` (unchanged in this PR). While we are doing formatting, is it legal in keyword arguments to have whitespace: `onclick:move = fn ...` ? If so, please also adopt such a convention, as it is far more readable. If not, I think the syntax design here really exposes the hypocrisy of the sanctimonious "oh whitespace-sensitive languages are so terrible and dangerous etc." because here we have a whitespace sensitivity that makes the programs less easily visually processed (and violate usual rules of punctuation, which include whitespace around =). I assume the state of affairs is the latter, maybe because a keyword argument needs to be distinguished from an in-line assignment, but using the lack of whitespace to do so when (I think) in other contexts an assignment _could_ elide the whitespace (or if not, why can you write `a+1` but not `a=1`)? But these fine points of whitespace-based semantics, at the cost of readability, seem nutty to me. Hopefully we can do better in Husht...
Vectornaut commented 2025-08-02 07:18:29 +00:00
Author
Member
Copy link

is it legal in keyword arguments to have whitespace

For clarity: Rust does not currently have keyword arguments. The code you're looking at is inside a Sycamore view! macro, which provides its own special synatx for laying out DOM nodes.

Sycamore props (as these node keyword arguments are called) can indeed have whitespace around the =. As you noted, this is consistent with Rust's general insensitivity to whitespace.

Sycamore's lead developer consistently writes props without space around the =, and the Sycamore community seems to broadly agree on this style, which is why I adopted it. This style seems widely used in languages that do have keyword arguments: for example, it's part of the Python style guide. I'm fine with using a different style, though, so I've added space around the = in commit b02e682.

> is it legal in keyword arguments to have whitespace For clarity: Rust does not [currently](https://github.com/rust-lang/rfcs/issues/323) have keyword arguments. The code you're looking at is inside a Sycamore [`view!`](https://sycamore.dev/book/introduction/your-first-app#creating-views) macro, which provides its own special synatx for laying out DOM nodes. Sycamore [*props*](https://sycamore.dev/book/introduction/your-first-app#component-props) (as these node keyword arguments are called) can indeed have whitespace around the `=`. As you noted, this is consistent with Rust's general insensitivity to whitespace. Sycamore's lead developer consistently writes props without space around the `=`, and the Sycamore community seems to broadly agree on this style, which is why I adopted it. This style seems widely used in languages that do have keyword arguments: for example, it's part of the [Python style guide](https://peps.python.org/pep-0008/#other-recommendations). I'm fine with using a different style, though, so I've added space around the `=` in commit b02e682.
glen commented 2025-08-02 23:51:52 +00:00
Owner
Copy link

Thanks for the clarification, and sounds good. I actually agree that in brief single-line calls,

foo(bar, key1=baz, key2=quux)

does actually look/read a bit better than

foo(bar, key1 = baz, key2 = quux)

but that when the value for the key is multi-line, the readability switches very strongly the other way. So I am fine with either always spacing them (if you prefer that consistency) or squeezing the spaces on single-line calls and leaving them in on multi-line calls -- whichever of these options you prefer.

Thanks for the clarification, and sounds good. I actually agree that in brief single-line calls, ``` foo(bar, key1=baz, key2=quux) ``` does actually look/read a bit better than ``` foo(bar, key1 = baz, key2 = quux) ``` but that when the value for the key is multi-line, the readability switches very strongly the other way. So I am fine with either always spacing them (if you prefer that consistency) or squeezing the spaces on single-line calls and leaving them in on multi-line calls -- whichever of these options you prefer.
Vectornaut commented 2025-08-04 03:19:43 +00:00
Author
Member
Copy link

Spacing around the = in single-line calls looks okay to me, and it's consistent with typical Rust style for things like the cfg attribute, so let's stick with the formatting from commit b02e682.

Spacing around the `=` in single-line calls looks okay to me, and it's consistent with typical Rust style for things like the [`cfg` attribute](https://doc.rust-lang.org/rust-by-example/attribute/cfg.html), so let's stick with the formatting from commit b02e682.
glen marked this conversation as resolved
app-proto/src/components/outline.rs
@ -181,2 +177,2 @@
},
_ => ()
}
_ => (),
glen commented 2025-08-01 22:15:39 +00:00
Owner
Copy link

this new comma looks particularly nutty when all of the other commas are being removed. Can we just leave it out, especially given that a null default return will presumably be forever the final branch here? Indeed, isn't the case that there could never be subsequent code here that would have any runtime effect, because the _ branch would always fire, cutting off any chance that subsequent branches would? This latter consideration obviates the need for the comma to avoid the "spurious git diff" phenomenon, since no useful code could ever be added subsequent to this branch. So please leave out this comma, thanks.

this new comma looks particularly nutty when all of the other commas are being removed. Can we just leave it out, especially given that a null default return will presumably be forever the final branch here? Indeed, isn't the case that there could never be subsequent code here that would have any runtime effect, because the `_` branch would always fire, cutting off any chance that subsequent branches would? This latter consideration obviates the need for the comma to avoid the "spurious git diff" phenomenon, since no useful code could ever be added subsequent to this branch. So please leave out this comma, thanks.
Vectornaut commented 2025-08-02 06:17:47 +00:00
Author
Member
Copy link

Now that we've switched to using commas after match arms wrapped in blocks, should I still remove the commas after catch-all match arms? That would make them the only match arms without commas, which makes them look weird to me.

Now that we've [switched](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3152) to using commas after match arms wrapped in blocks, should I still remove the commas after catch-all match arms? That would make them the only match arms without commas, which makes them look weird to me.
glen commented 2025-08-02 23:52:41 +00:00
Owner
Copy link

Yes, agreed, if we are always using commas, they should be here too. Resolving.

Yes, agreed, if we are always using commas, they should be here too. Resolving.
glen marked this conversation as resolved
app-proto/src/components/test_assembly_chooser.rs
@ -918,7 +918,7 @@ pub fn TestAssemblyChooser() -> View {
"off-center" => load_off_center_assemb(assembly),
glen commented 2025-08-01 22:24:55 +00:00
Owner
Copy link

Please either:

  • Put the "ly"s into the last word of these function names -- there's no real value in using a non-word abbreviation for a plain word when it only saves two characters, or
  • elide the _assemb part, and quite possibly all the commonload_ parts, of these function names altogether. I'd find
    "off-center" => off_center(assembly), ...

no problem to read, and in any case it seems that

    "off-center" => load_off_center(assembly), ...

should certainly suffice if you find just "off_center" to be a bit too compressed.

I prefer some variation of the second bullet, as you might imagine.

Please either: * Put the "ly"s into the last word of these function names -- there's no real value in using a non-word abbreviation for a plain word when it only saves two characters, or * elide the `_assemb` part, and quite possibly all the common`load_` parts, of these function names altogether. I'd find ``` "off-center" => off_center(assembly), ... ``` no problem to read, and in any case it seems that ``` "off-center" => load_off_center(assembly), ... ``` should certainly suffice if you find just "off_center" to be a bit too compressed. I prefer some variation of the second bullet, as you might imagine.
Vectornaut commented 2025-08-02 07:22:44 +00:00
Author
Member
Copy link

elide the _assemb part, and quite possibly all the common load_ parts, of these function names altogether

I agree that dropping the _assemb suffix makes sense now that we've split the test assembly chooser off into its own module. I've added this to my list of simplifications that are more likely to affect the abstract syntax tree, and therefore more appropriate for a separate PR.

> elide the `_assemb` part, and quite possibly all the common `load_` parts, of these function names altogether I agree that dropping the `_assemb` suffix makes sense now that we've split the test assembly chooser off into its own module. I've added this to my list of simplifications that are more likely to affect the abstract syntax tree, and therefore more appropriate for a separate PR.
glen commented 2025-08-02 23:56:08 +00:00
Owner
Copy link

I don't personally see any problem with folding wholesale identifier substitutions (every baz becomes quux, for example) into this PR, but if you prefer to do it in a separate PR immediately following this one, that's fine too. We should just get through all the current "typographical" changes before returning to semantic ones...

I don't personally see any problem with folding wholesale identifier substitutions (every baz becomes quux, for example) into this PR, but if you prefer to do it in a separate PR immediately following this one, that's fine too. We should just get through all the current "typographical" changes before returning to semantic ones...
Vectornaut commented 2025-08-04 03:20:44 +00:00
Author
Member
Copy link

Okay, let's do that in the next pull request.

Okay, let's do that in the next pull request.
glen commented 2025-08-04 09:01:07 +00:00
Owner
Copy link

OK, please file an issue for it so we don't forget. Resolving this thread.

OK, please file an issue for it so we don't forget. Resolving this thread.
Vectornaut commented 2025-08-04 21:45:42 +00:00
Author
Member
Copy link

This is now issue #109.

This is now issue #109.
glen marked this conversation as resolved
app-proto/src/components/test_assembly_chooser.rs
@ -920,3 +920,3 @@
"irisawa-hexlet" => load_irisawa_hexlet_assemb(assembly),
_ => ()
_ => (),
};
glen commented 2025-08-01 22:20:52 +00:00
Owner
Copy link

this comma looks more uniform, but hopefully we can get rid of all of the commas? In fact, if commas are at the moment required in standard Rust syntax when the branch is an expression, I might even prefer (until Husht helps us curtail brace-o-rama)

   "off-center" => {load_off_center_assemb(assembly)}
   "radius-ratio" => {load_radius_ratio_assemb(assembly)}
   _ => ()

What do you think?

this comma looks more uniform, but hopefully we can get rid of all of the commas? In fact, if commas are at the moment required in standard Rust syntax when the branch is an expression, I might even prefer (until Husht helps us curtail brace-o-rama) ``` "off-center" => {load_off_center_assemb(assembly)} "radius-ratio" => {load_radius_ratio_assemb(assembly)} _ => () ``` What do you think?
Vectornaut commented 2025-08-02 07:25:45 +00:00
Author
Member
Copy link

What do you think?

I think that commas on every arm (as we now have) are much more readable than braces on every arm.

> What do you think? I think that commas on every arm (as we [now have](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3152)) are much more readable than braces on every arm.
glen marked this conversation as resolved
app-proto/src/engine.rs
@ -396,3 +394,3 @@
);
return Realization { result, history }
return Realization { result, history };
}
glen commented 2025-08-01 22:33:17 +00:00
Owner
Copy link

Why is an explicit return needed here, rather than just a final expression? And why the semicolon now -- is it added just because of the return, or is a no-spurious-git-diff thing, too, because it would be necessary if another statement were added? But as that's impossible since code after an unqualified return can't be executed, can we leave out the semicolon even if there's a reason return is needed? Thanks for clarifying.

Why is an explicit return needed here, rather than just a final expression? And why the semicolon now -- is it added just because of the return, or is a no-spurious-git-diff thing, too, because it would be necessary if another statement were added? But as that's impossible since code after an unqualified return can't be executed, can we leave out the semicolon even if there's a reason return is needed? Thanks for clarifying.
Vectornaut commented 2025-08-02 08:14:46 +00:00
Author
Member
Copy link

why the semicolon now

The semicolon helps communicate the intent of the code, as explained below. I'm adding it now because I didn't notice it was missing until now, when I ran Rustfmt in check mode and saw its absence flagged as a formatting error.

Why is an explicit return needed here, rather than just a final expression?

Let's look at a simpler example.

fn factorial(n: u128) -> u128 {
    if n == 0 {
        return 1;
    }
    n * factorial(n-1)
}

The expression

if n == 0 {
    return 1;
}

has the unit value (), which is discarded. It also has the important side effect of causing the function to stop and return 1 when n == 0. The trailing semicolon emphasizes that the important thing about this expression is its side effect, not its value.

The alternate expression

if n == 0 {
    return 1
}

has the same value and side effect. However, it gets its value in a different and more confusing way: it evalutes to the value of the expression return 1, which happens to be (). When the last expression in a block has no semicolon, that expression lends its value to the block, so leaving the semicolon off the last expression suggests (misleadingly, in this case) that its value is important.

The alternate expression

if n == 0 {
    1
}

gives a type error, because an if with a non-unit value has to come with an else that evaluates to the same type.

In our simple example, we could rewrite the whole function like this, which I find easier to understand:

fn factorial(n: u128) -> u128 {
    if n == 0 {
        1
    } else {
        n * factorial(n-1)
    }
}

If we rewrote realize_gram like this, however, we'd end up with over a hundred lines of code inside the else block, which I think would make the whole function less readable.

> why the semicolon now The semicolon helps communicate the intent of the code, as explained below. I'm adding it now because I didn't notice it was missing until now, when I ran Rustfmt in check mode and saw its absence flagged as a formatting error. > Why is an explicit return needed here, rather than just a final expression? Let's look at a simpler example. ```rust fn factorial(n: u128) -> u128 { if n == 0 { return 1; } n * factorial(n-1) } ``` The expression ```rust if n == 0 { return 1; } ``` has the unit value `()`, which is discarded. It also has the important side effect of causing the function to stop and return `1` when `n == 0`. The trailing semicolon emphasizes that the important thing about this expression is its side effect, not its value. The alternate expression ```rust if n == 0 { return 1 } ``` has the same value and side effect. However, it gets its value in a different and more confusing way: it evalutes to the value of the expression `return 1`, which happens to be `()`. When the last expression in a block has no semicolon, that expression lends its value to the block, so leaving the semicolon off the last expression suggests (misleadingly, in this case) that its value is important. The alternate expression ```rust if n == 0 { 1 } ``` gives a type error, because an `if` with a non-unit value has to come with an `else` that evaluates to the same type. In our simple example, we could rewrite the whole function like this, which I find easier to understand: ```rust fn factorial(n: u128) -> u128 { if n == 0 { 1 } else { n * factorial(n-1) } } ``` If we rewrote `realize_gram` like this, however, we'd end up with over a hundred lines of code inside the `else` block, which I think would make the whole function less readable.
glen commented 2025-08-03 17:10:37 +00:00
Owner
Copy link

OK, so let me see if I understand -- there is a return here because it is not the end of a function, but just of an if-branch; and you are using a semicolon to signify that the return value of the return statement is ignored. I am not a fan of the latter, but I am not going to worry about it at all as semicolons should be handled in a mutually agreeable way in Husht. So resolving.

OK, so let me see if I understand -- there is a return here because it is not the end of a function, but just of an if-branch; and you are using a semicolon to signify that the return value of the return statement is ignored. I am not a fan of the latter, but I am not going to worry about it at all as semicolons should be handled in a mutually agreeable way in Husht. So resolving.
👍 1
glen marked this conversation as resolved
app-proto/src/engine.rs
@ -486,3 +484,3 @@
if let Some((better_state, backoff_steps)) = seek_better_config(
gram, &state, &base_step, neg_grad.dot(&base_step),
min_efficiency, backoff, max_backoff_steps
min_efficiency, backoff, max_backoff_steps,
glen commented 2025-08-01 22:37:30 +00:00
Owner
Copy link

OK, so here's an interesting point: here you include an arbitrary varying number of arguments on each line of the call, grouped/formatted as you see fit for compactness/readability, whereas other places function declarations and calls got exploded to single parameters/arguments per line. I am actually a fan of the approach here, of coder's discretion as to how many parameters/arguments go on each line on a local basis for compactness/readability. Should/could we use multiple parameters/arguments on one line in some of the other places to beneficial effect? I am not recommending this location be changed, and I am in favor of the new final comma.

OK, so here's an interesting point: here you include an arbitrary varying number of arguments on each line of the call, grouped/formatted as you see fit for compactness/readability, whereas other places function declarations and calls got exploded to single parameters/arguments per line. I am actually a fan of the approach here, of coder's discretion as to how many parameters/arguments go on each line on a local basis for compactness/readability. Should/could we use multiple parameters/arguments on one line in some of the other places to beneficial effect? I am not recommending this location be changed, and I am in favor of the new final comma.
Vectornaut commented 2025-08-02 08:18:47 +00:00
Author
Member
Copy link

That sounds fine. I've suggested some groupings for SceneSpheres::push and ScenePoints::push. Are there any other function declarations that you'd like to see grouped in this way?

That sounds fine. I've [suggested](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/108#issuecomment-3155) some groupings for `SceneSpheres::push` and `ScenePoints::push`. Are there any other function declarations that you'd like to see grouped in this way?
glen commented 2025-08-03 17:12:29 +00:00
Owner
Copy link

Not specifically that I am seeing at the moment. Just want to establish such grouping as a reasonable practice, rather than some other style guides that insist on exactly one argument per line as soon as the entire call is not on one line (such as the one we are using for Numberscope, which bugs me).
So resolving.

Not specifically that I am seeing at the moment. Just want to establish such grouping as a reasonable practice, rather than some other style guides that insist on exactly one argument per line as soon as the entire call is not on one line (such as the one we are using for Numberscope, which bugs me). So resolving.
👍 1
glen marked this conversation as resolved
Vectornaut added 1 commit 2025-08-02 05:40:12 +00:00
Collapse a structure expression onto one line
All checks were successful
/ test (pull_request) Successful in 3m34s
Details
ebad512a03 
I had put `active_tab: /* ... */` on its own line to make the
`create_signal` call more readable, but it wasn't worth the cost in
vertical space.
Vectornaut added 1 commit 2025-08-02 06:03:33 +00:00
Add commas after match arms wrapped in blocks
All checks were successful
/ test (pull_request) Successful in 3m29s
Details
bfd5d8e35f
Vectornaut added 1 commit 2025-08-02 07:16:08 +00:00
Add space around = for Sycamore props
All checks were successful
/ test (pull_request) Successful in 3m35s
Details
b02e682e15 
Also, add a trailing comma to a match arm in `outline.rs`.
glen commented 2025-08-03 17:13:47 +00:00
Owner
Copy link

OK, I have gotten through all your feedback, and resolved all the items except for ones waiting for some further opinion on your end. Thanks.

OK, I have gotten through all your feedback, and resolved all the items except for ones waiting for some further opinion on your end. Thanks.
Vectornaut added 1 commit 2025-08-04 03:09:59 +00:00
Group the parameters of the scene push methods
All checks were successful
/ test (pull_request) Successful in 3m39s
Details
e67a658e00
glen commented 2025-08-04 23:31:35 +00:00
Owner
Copy link

OK, except for the couple of things slated for the immediately next PR, looks fine and everything seemed to run OK on my end, so merging.

OK, except for the couple of things slated for the immediately next PR, looks fine and everything seemed to run OK on my end, so merging.
glen merged commit ef1a579ac0 into main 2025-08-04 23:34:35 +00:00
glen referenced this pull request from a commit 2025-08-04 23:34:37 +00:00
refactor: Code formatting (#108)
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
design

Issues that entail architecture/design decisions

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
maintenance

refactors andother behavior-orthogonal tasks

  
prospective

this becomes an issue once a particular feature is implemented or pull request is merged

  
question

More information is needed

  
regression

A bug that's resurfaced or a feature that's accidentally been removed

  
stub

We need to fill in the details of this issue

  
todo

Planned step in implementation

  
ui

issue concerning the user interface

  
wontfix

This won't be fixed

No labels
bug
design
duplicate
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: StudioInfinity/dyna3#108
No description provided.
Delete branch "Vectornaut/dyna3:trailing-commas"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?