chore: wrap at 80 characters #128

Open
glen wants to merge 3 commits from glen/dyna3:aroundLineInEightyChars into main
Owner

This is built on top of #126. It simply wraps all lines in app-proto/src at 80 characters.

This is built on top of #126. It simply wraps all lines in app-proto/src at 80 characters.
glen added 9 commits 2025-10-11 05:36:13 +00:00
feat: Point coordinate regulators
All checks were successful
/ test (pull_request) Successful in 3m35s
ecbbe2068c
Implements regulators for the Euclidean coordinates of Point entities,
  automatically creating all three of them for each added point entity. When
  such a regulator is set, it freezes the corresponding representation
  coordinate to the set point. In addition, if all three coordinates of a
  given Point are set, the coradius coordinate (which holds the norm of the
  point) is frozen as well.

  Note that a PointCoordinateRegulator must be created with a Point as the
  subject. This commit modifies HalfCurvatureRegulator analogously, so that
  it can only be created with a Sphere.

  A couple of prospective issues that should be filed in association with
  this commit:
  * The new coordinate regulators create redundant display information with
    the raw representation coordinates of a point that are already shown in
    the outline view.
  * The optimization status of these regulators together with HalfCurvature
    regulators (i.e., the ones implemented by freezing coordinates) is different
    from InversiveDistance regulators when an Assembly is unrealizable: the
    frozen-coordinate constraints will be "hard" in that they will be forced
    to precisely equal their set point, whereas the distance regulators are
    "soft" in that they can be relaxed from their set points in an effort to
    minimize the loss function of the configuration as compared to the values
    of the constraints. Perhaps at some point we should/will have a mechanism
    to specify the softness/hardness of constraints, but in the meantime,
    there should not be two different categories of constraints. Suppose we
    decide that by default that all constraints are soft. Then the optimizer
    should be able to search changing, for example, the radius of a
    curvature-constrained sphere, so as to minimize the loss function (for a
    loss that would therefore presumably have a term akin to the square of the
    difference between the specified and actual half-curvature of the sphere).
    For example, suppose you specify that the half-curvature of a sphere is 1
    (so it has radius 1/2) but that its distance to a point is -1. These
    constraints cannot be satisfied, so the optimization fails, presumably
    with the point at the sphere center, and the sphere with radius 1/2.
    So all of the loss is concentrated in the difference between the actual
    point-sphere distance being -1/2, not -1. It would be more appropriate
    (in the all-soft constraint regime) to end up at something like a sphere of
    half-curvature 1/√2 with the point at the center, so that the loss is split
    between both the half-curvature and the distance to the sphere being off by
    1 - 1/√2. (At a guess, that would minimize the sum of the squares of the
    two differences.)
chore: typographical improvements per review
All checks were successful
/ test (pull_request) Successful in 3m45s
46ffd6c285
This makes it simpler, from the programmer's perspective, to get the
name of an axis as a string slice and to format an axis name into a
string. To me, the matching method `Axis::name` seems more direct than
the explicit lookup table that it replaces, and I'm hoping that it will
be about as easy for the compiler to inline, or even easier.

Implementing `Display` enables us to hand an `Axis` to a string
formatter without any explicit conversion. It adds extra code in the
short run, but I'd expect it to simplify our code in the long run by
fitting into the conventions set by the Rust standard library.
Spruce up formatting and error messages
All checks were successful
/ test (pull_request) Successful in 3m43s
adc60ac5c1
Make the new code's formatting and error messages more consistent with
the previous code. I don't necessarily have a strong preference for the
previous conventions, but I do like stuff to be consistent.
Revert "Spruce up formatting and error messages"
All checks were successful
/ test (pull_request) Successful in 3m40s
c081f1a809
This reverts commit adc60ac5c1. We decided
that it would be better for me to request formatting changes one by one.
chore: uniformize error messages, 80-char lines, import fmt::Display
All checks were successful
/ test (pull_request) Successful in 3m48s
c0e6ebf3d6
chore: revert 80-character limit, reorder and rename index message helper
All checks were successful
/ test (pull_request) Successful in 3m38s
50c51ca08f
chore: remove trailing whitespace, add CR at end of file
All checks were successful
/ test (pull_request) Successful in 3m36s
3405797d14
chore:wrap lines at 80 characters
All checks were successful
/ test (pull_request) Successful in 3m39s
2f6c93ec42
glen force-pushed aroundLineInEightyChars from 2f6c93ec42 to 19e5458e1b 2025-10-11 06:00:45 +00:00 Compare
glen force-pushed aroundLineInEightyChars from 19e5458e1b to dde499b736 2025-10-14 19:30:54 +00:00 Compare
glen force-pushed aroundLineInEightyChars from dde499b736 to fae94486d6 2025-10-14 19:45:18 +00:00 Compare
Author
Owner

This should now be properly rebased on #129.

This should now be properly rebased on #129.
glen force-pushed aroundLineInEightyChars from fae94486d6 to 4a3f47c8b5 2025-11-11 23:22:02 +00:00 Compare
Author
Owner

Now rebased onto main. @Vectornaut had a couple of in-person comments that I don't remember the details of, which could be posted now, before or in conjunction with a full review.

Now rebased onto main. @Vectornaut had a couple of in-person comments that I don't remember the details of, which could be posted now, before or in conjunction with a full review.
Vectornaut requested changes 2025-11-13 06:22:22 +00:00
Vectornaut left a comment
Member

Aside from two changes that correct naming errors, which I've flagged with an ️ at the beginning of each comment, all of my suggestions are stylistic.

When a parenthesized expression is split over two lines, I still prefer something like the first option below rather than the second.

let sphere = assembly.elements_by_id.with(
    |elts_by_id| elts_by_id[sphere_id].clone()
);
let sphere = assembly.elements_by_id.with(
    |elts_by_id| elts_by_id[sphere_id].clone());

However, I think you've already said you strongly prefer the second option, which is used in most of the pull request. If so, there's no need to discuss it further.

Aside from two changes that correct naming errors, which I've flagged with an ❌️ at the beginning of each comment, all of my suggestions are stylistic. When a parenthesized expression is split over two lines, I still prefer something like the first option below rather than the second. ```rust let sphere = assembly.elements_by_id.with( |elts_by_id| elts_by_id[sphere_id].clone() ); ``` ```rust let sphere = assembly.elements_by_id.with( |elts_by_id| elts_by_id[sphere_id].clone()); ``` However, I think you've already said you strongly prefer the second option, which is used in most of the pull request. If so, there's no need to discuss it further.
@ -359,2 +368,3 @@
const FOCAL_SLOPE: f64 = 0.3;
let horizontal = f64::from(event.client_x()) - rect.left();
let vertical = rect.bottom() - f64::from(event.client_y());
Member

I think names like x_relative, y_relative or x_rel, y_rel would be more descriptive, and more analogous with the original values event.client_x(), event.client_y(), than the current names horizontal, vertical.

I think names like `x_relative`, `y_relative` or `x_rel`, `y_rel` would be more descriptive, and more analogous with the original values `event.client_x()`, `event.client_y()`, than the current names `horizontal`, `vertical`.
Author
Owner

Renamed to x_relative, y_relative.

Renamed to x_relative, y_relative.
Vectornaut marked this conversation as resolved
@ -918,1 +960,3 @@
match assembly_to_world.with(|asm_to_world| elt.cast(dir, asm_to_world, pixel_size)) {
let target = assembly_to_world.with(
|asm_to_world| elt.cast(dir, asm_to_world, pixel_size));
match target {
Member

️ I'd call this value something like hit_depth rather than target, because it doesn't tell you what you hit: it only tells you whether you hit elt, and how far out along the click ray you hit it if you did.

❌️ I'd call this value something like `hit_depth` rather than `target`, because it doesn't tell you what you hit: it only tells you whether you hit `elt`, and how far out along the click ray you hit it if you did.
Author
Owner

Renamed to just hit, as the depth is immediately extracted on the next line, and then the None branch makes more sense: no hit at all, not just a missing depth of a hit.

Renamed to just `hit`, as the depth is immediately extracted on the next line, and then the `None` branch makes more sense: no hit at all, not just a missing depth of a hit.
Member

In ray-casting, I'd typically use hit to denote the full coordinates of the hit point. If that changes your mind about the clarity of hit here, we can workshop a different name. If you still think hit is clear enough here, I'm willing to accept it.

In ray-casting, I'd typically use `hit` to denote the full coordinates of the hit point. If that changes your mind about the clarity of `hit` here, we can workshop a different name. If you still think `hit` is clear enough here, I'm willing to accept it.
Author
Owner

check_hit?

check_hit?
Member

How about depth_if_hit? To me, this communicates that the value will be Some(depth) if we hit elt and None if we missed.

How about `depth_if_hit`? To me, this communicates that the value will be `Some(depth)` if we hit `elt` and `None` if we missed.
Author
Owner

since the method ultimately being called is named cast, I just called the result cast.

since the method ultimately being called is named `cast`, I just called the result `cast`.
Vectornaut marked this conversation as resolved
@ -221,0 +227,4 @@
let x32 = x as f32;
let y32 = y as f32;
let coords =
[0.5*(1.0 + x32), 0.5*(1.0 + y32), 0.5*(1.0 - x32*y32)];
Member

️ This is the sphere's color, not its representation coordinates. I'd call it color, matching the name of the Sphere::new argument that it gets plugged into later.

❌️ This is the sphere's color, not its representation coordinates. I'd call it `color`, matching the name of the `Sphere::new` argument that it gets plugged into later.
Author
Owner

Renamed to color.

Renamed to color.
Vectornaut marked this conversation as resolved
@ -509,2 +533,3 @@
// set up the tangencies that define the packing
for [long_edge_plane, short_edge_plane] in [["a", "b"], ["b", "c"], ["c", "a"]] {
for [long_edge_plane, short_edge_plane]
in [["a", "b"], ["b", "c"], ["c", "a"]] {
Member

I strongly prefer to put the opening { at the same indentation level as the for, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Either of the following would look fine to me.

for [long_edge_plane, short_edge_plane]
    in [["a", "b"], ["b", "c"], ["c", "a"]]
{
    /* loop contents */
}
for [long_edge_plane, short_edge_plane] in [
    ["a", "b"], ["b", "c"], ["c", "a"]
] {
    /* loop contents */
}
I strongly prefer to put the opening `{` at the same indentation level as the `for`, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Either of the following would look fine to me. ```rust for [long_edge_plane, short_edge_plane] in [["a", "b"], ["b", "c"], ["c", "a"]] { /* loop contents */ } ``` ```rust for [long_edge_plane, short_edge_plane] in [ ["a", "b"], ["b", "c"], ["c", "a"] ] { /* loop contents */ } ```
Author
Owner

Tried to recast all of these accordingly. This is something we will have to give some thought to in Husht, how to delimit a multiline loop control portion from the loop body, if there are no braces.

Tried to recast all of these accordingly. This is something we will have to give some thought to in Husht, how to delimit a multiline loop control portion from the loop body, if there are no braces.
Member

The Python style guide has some recommendations for how to deal with this in if statements.

The [Python style guide](https://peps.python.org/pep-0008/#indentation) has some recommendations for how to deal with this in `if` statements.
Author
Owner

Right well lookjng there you find three options. One is do nothing, which necessitates a delimiter between condition and consequent, which doesn't seem feasible for husht (get rid of braces just to introduce a new delimiter?), another is introduce a comment, but i think we both dislike semantically significant comments ;-), and the third is the extra-indent-in-condition convention i tried but you didn't like. Hence how to do this in Husht will require real thought.

Right well lookjng there you find three options. One is do nothing, which necessitates a delimiter between condition and consequent, which doesn't seem feasible for husht (get rid of braces just to introduce a new delimiter?), another is introduce a comment, but i think we both dislike semantically significant comments ;-), and the third is the extra-indent-in-condition convention i tried but you didn't like. Hence how to do this in Husht will require real thought.
Author
Owner

I checked the civet documentation and based on that here are two options, not mutually exclusive, nor would have to go the same way for both conditionals and loops (civet uses the first for loops and the second for conditionals):

  1. Enclose a long condition/control in parentheses:
if (this_thing
     && that_thing)
    do_stuff()
always_do_this()
for (item
    in long_list)
    operate_on(item)
after_loop()
  1. Introduce an optional separator keyword which is always allowed but is necessary either if whole statement is on one line or if condition/control is on multiple lines:
if this_thing
    && that_thing
then do_stuff()
for item
    in long_list
do opererate_on(item)

Seems like some combination of these schemes would work for husht.

I checked the civet documentation and based on that here are two options, not mutually exclusive, nor would have to go the same way for both conditionals and loops (civet uses the first for loops and the second for conditionals): 1. Enclose a long condition/control in parentheses: ``` if (this_thing && that_thing) do_stuff() always_do_this() ``` ``` for (item in long_list) operate_on(item) after_loop() ``` 2. Introduce an optional separator keyword which is always allowed but is necessary either if whole statement is on one line or if condition/control is on multiple lines: ``` if this_thing && that_thing then do_stuff() ``` ``` for item in long_list do opererate_on(item) ``` Seems like some combination of these schemes would work for husht.
Vectornaut marked this conversation as resolved
@ -862,2 +898,3 @@
);
for (chain_sphere, chain_sphere_next) in chain.clone().zip(chain.cycle().skip(1)) {
for (chain_sphere, chain_sphere_next)
in chain.clone().zip(chain.cycle().skip(1)) {
Member

I strongly prefer to put the opening { at the same indentation level as the for, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Here's an example.

for (chain_sphere, chain_sphere_next)
    in chain.clone().zip(chain.cycle().skip(1))
{
    /* loop contents */
}
I strongly prefer to put the opening `{` at the same indentation level as the `for`, allowing the loop contents to appear at the expected indentation level without double-indenting anything. Here's an example. ```rust for (chain_sphere, chain_sphere_next) in chain.clone().zip(chain.cycle().skip(1)) { /* loop contents */ } ```
Author
Owner

This is a ditto. There were several others as well.

This is a ditto. There were several others as well.
Member

There were several others as well.

Yes, looks like you got them all!

> There were several others as well. Yes, looks like you got them all!
Vectornaut marked this conversation as resolved
@ -26,1 +29,3 @@
pub fn sphere_with_offset(dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64) -> DVector<f64> {
pub fn sphere_with_offset(
dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64) -> DVector<f64>
{
Member

Personally, I prefer the following organization, which takes up the same number of lines.

pub fn sphere_with_offset(
    dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64
) -> DVector<f64> {

I prefer it despite its potential inconsistency with the first of the following options for splitting function calls, which does save a line and which is used elsewhere in this pull request.

function_name(
    arg1, arg2, arg3, arg4);
function_name(
    arg1, arg2, arg3, arg4
);

If you strongly prefer consistency, though, I'll defer to that.

This comment also applies to several other function declarations, which I can highlight if you accept the change.

Personally, I prefer the following organization, which takes up the same number of lines. ```rust pub fn sphere_with_offset( dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f64 ) -> DVector<f64> { ``` I prefer it despite its potential inconsistency with the first of the following options for splitting function calls, which does save a line and which is used elsewhere in this pull request. ```rust function_name( arg1, arg2, arg3, arg4); ``` ```rust function_name( arg1, arg2, arg3, arg4 ); ``` If you strongly prefer consistency, though, I'll defer to that. This comment also applies to several other function declarations, which I can highlight if you accept the change.
Author
Owner

Tried to go through and reformat accordingly. This may also be something that requires thought in Husht, along with multi-line "if" conditions.

Tried to go through and reformat accordingly. This may also be something that requires thought in Husht, along with multi-line "if" conditions.
Member

Tried to go through and reformat accordingly.

Thanks! I've flagged a few missed occurrences in the next review.

> Tried to go through and reformat accordingly. Thanks! I've flagged a few missed occurrences in the next review.
Vectornaut marked this conversation as resolved
@ -512,1 +526,3 @@
Ok(ConfigNeighborhood { #[cfg(feature = "dev")] config: state.config, nbhd: tangent })
Ok(ConfigNeighborhood {
#[cfg(feature = "dev")] config: state.config, nbhd: tangent
})
Member

If we're going to split this over multiple lines, I think it's worth an extra line to make the structure more readable.

Ok(ConfigNeighborhood {
    #[cfg(feature = "dev")] config: state.config,
    nbhd: tangent,
})
If we're going to split this over multiple lines, I think it's worth an extra line to make the structure more readable. ```rust Ok(ConfigNeighborhood { #[cfg(feature = "dev")] config: state.config, nbhd: tangent, }) ```
Author
Owner

Reformatted as suggested

Reformatted as suggested
Vectornaut marked this conversation as resolved
@ -941,0 +971,4 @@
let Realization { result: result_tfm, history: history_tfm } =
realize_gram(&problem_tfm, SCALED_TOL, 0.5, 0.9, 1.1, 200, 110);
let ConfigNeighborhood { config: config_tfm, nbhd: tangent_tfm } =
result_tfm.unwrap();
Member

Another option, which I find more readable:

let ConfigNeighborhood {
    config: config_tfm,
    nbhd: tangent_tfm,
} = result_tfm.unwrap();
Another option, which I find more readable: ```rust let ConfigNeighborhood { config: config_tfm, nbhd: tangent_tfm, } = result_tfm.unwrap(); ```
Author
Owner

Wasn't quite clear about what the layout/readability issue was here. I definitely prefer keeping the entity being assigned to (it's unfortunate it's just so verbose here) as compact as possible. I tried a different tweak.

Wasn't quite clear about what the layout/readability issue was here. I definitely prefer keeping the entity being assigned to (it's unfortunate it's just so verbose here) as compact as possible. I tried a different tweak.
Member

Your alternate tweak looks fine to me.

Your alternate tweak looks fine to me.
Vectornaut marked this conversation as resolved
Author
Owner

@Vectornaut wrote in #128 (comment):

When a parenthesized expression is split over two lines, I still prefer something like the first option below rather than the second.

let sphere = assembly.elements_by_id.with(
    |elts_by_id| elts_by_id[sphere_id].clone()
);
let sphere = assembly.elements_by_id.with(
   |elts_by_id| elts_by_id[sphere_id].clone());

However, I think you've already said you strongly prefer the second option, which is used in most of the pull request. If so, there's no need to discuss it further.

Well, it pretty well corresponds: I like letting the indentation show the grouping and I prefer not to use up a vertical line solely for punctuation, whereas I gather you are very comfortable with the predominant style of putting closing punctuation marks alone (or sometimes in small combinations) on a line at the same indentation as the line on which the opening punctuation marks occurred (regardless of where on that line the opening mark appeared, typically at the end of that earlier line. (And also that parenthesis all by itself, forlorn on its own line, is so far from how we punctuate ordinary discourse that it just looks to me jarring and dangle-y.)

A reasonable question is how will this be handled by Husht, presuming I can eventually carve out time to move that further? I don't think we've at any point contemplated doing away with function-call parentheses, so this would presumably (run all onto one line) end up as something like

let sphere = assembly.elements_by_id.with(elts_by_id => elts_by_id[sphere_id].clone())

or in other words, the actual code won't change much (I mean, we could at some point add convenience unary-function-writing features like Civet has to end up with something like

let sphere = assembly.elements_by_id.with(_[sphere_id].clone())

but that would be a significantly down-the-road addition that's not really relevant at the moment, other than to note that the savings of not naming the sole parameter of the function argument fits the whole snippet in 80 characters.)


OK, so even in currently foreseeable Husht, we will have to break the line, and so may have a quandary concerning where the close goes. We should find a mutually comfortable way to handle such cases, insofar as possible. Brainstorming:

A) Should we prefer linebreaks that keep open and close punctuation on the same line when possible? For example, write this
particular snippet as

let sphere = assembly.elements_by_id
    .with(|elts_by_id| elts_by_id[sphere_id].clone());

B) Should we leave this item to the writer's discretion and make a gentleperson's agreement not to change instances of it that that the other wrote except when there is a true need to otherwise adjust such code? Recall I am (somewhat radically in the current era) of the opinion that a code writer should use the latitude afforded by the language of choice to express the code as clearly as they can, and that best expression may well in particular cases not be whatever some rote set of codebot rules would generate.

So the short answer is yes, I do much prefer

let x = pretend(this_function _call,
    is_too_long_for_one_line);

and/or

let x = pretend(
    this_function _call, is_too_long_for_one_line);

to either

let x = pretend(this_function _call,
    is_too_long_for_one_line
);

or

let x = pretend(
    this_function _call,
    is_too_long_for_one_line
);

But it is also very important to me that you're comfortable with our coding practices. So let me know your thoughts and when we have settled, I will adjust the instances touched in this PR.
Thanks!

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3430: > When a parenthesized expression is split over two lines, I still prefer something like the first option below rather than the second. > > ```rust > let sphere = assembly.elements_by_id.with( > |elts_by_id| elts_by_id[sphere_id].clone() > ); > ``` > ``` > let sphere = assembly.elements_by_id.with( > |elts_by_id| elts_by_id[sphere_id].clone()); > ``` > > However, I think you've already said you strongly prefer the second option, which is used in most of the pull request. If so, there's no need to discuss it further. Well, it pretty well corresponds: I like letting the indentation show the grouping and I prefer not to use up a vertical line solely for punctuation, whereas I gather you are very comfortable with the predominant style of putting closing punctuation marks alone (or sometimes in small combinations) on a line at the same indentation as the line on which the opening punctuation marks occurred (regardless of where on that line the opening mark appeared, typically at the end of that earlier line. (And also that parenthesis all by itself, forlorn on its own line, is so far from how we punctuate ordinary discourse that it just looks to me jarring and dangle-y.) A reasonable question is how will this be handled by Husht, presuming I can eventually carve out time to move that further? I don't think we've at any point contemplated doing away with function-call parentheses, so this would presumably (run all onto one line) end up as something like ``` let sphere = assembly.elements_by_id.with(elts_by_id => elts_by_id[sphere_id].clone()) ``` or in other words, the actual code won't change much (I mean, we could at some point add convenience unary-function-writing features like Civet has to end up with something like ``` let sphere = assembly.elements_by_id.with(_[sphere_id].clone()) ``` but that would be a significantly down-the-road addition that's not really relevant at the moment, other than to note that the savings of not naming the sole parameter of the function argument fits the whole snippet in 80 characters.) ----- OK, so even in currently foreseeable Husht, we will have to break the line, and so may have a quandary concerning where the close goes. We should find a mutually comfortable way to handle such cases, insofar as possible. Brainstorming: A) Should we prefer linebreaks that keep open and close punctuation on the same line when possible? For example, write this particular snippet as ``` let sphere = assembly.elements_by_id .with(|elts_by_id| elts_by_id[sphere_id].clone()); ``` B) Should we leave this item to the writer's discretion and make a gentleperson's agreement not to change instances of it that that the other wrote except when there is a true need to otherwise adjust such code? Recall I am (somewhat radically in the current era) of the opinion that a code writer should use the latitude afforded by the language of choice to express the code as clearly as they can, and that best expression may well in particular cases not be whatever some rote set of codebot rules would generate. So the short answer is yes, I do much prefer ``` let x = pretend(this_function _call, is_too_long_for_one_line); ``` and/or ``` let x = pretend( this_function _call, is_too_long_for_one_line); ``` to either ``` let x = pretend(this_function _call, is_too_long_for_one_line ); ``` or ``` let x = pretend( this_function _call, is_too_long_for_one_line ); ``` But it is also very important to me that you're comfortable with our coding practices. So let me know your thoughts and when we have settled, I will adjust the instances touched in this PR. Thanks!
Member

Should we prefer linebreaks that keep open and close punctuation on the same line when possible? For example, write this
particular snippet as

let sphere = assembly.elements_by_id
    .with(|elts_by_id| elts_by_id[sphere_id].clone());

I would certainly prefer it, because it's consistent with the Rust code I'm used to reading.

Should we leave this item to the writer's discretion and make a gentleperson's agreement not to change instances of it that that the other wrote except when there is a true need to otherwise adjust such code?

That seems fine to me.

A reasonable question is how will this be handled by Husht, presuming I can eventually carve out time to move that further?

Since we plan for Husht to have Python-like syntax, I think it would make sense to write it in a more Python-like style. The Python style guide is consistent with the style you're using for long function calls:

  • First line has the function name and opening parenthesis (no arguments).
  • Subsequent lines have arguments and are indented one level.
  • Last line includes both arguments and closing parenthesis.
let sphere = assembly.elements_by_id.with(
    |elts_by_id| elts_by_id[sphere_id].clone())
> Should we prefer linebreaks that keep open and close punctuation on the same line when possible? For example, write this particular snippet as > ```rust > let sphere = assembly.elements_by_id > .with(|elts_by_id| elts_by_id[sphere_id].clone()); > ``` I would certainly prefer it, because it's consistent with the Rust code I'm used to reading. > Should we leave this item to the writer's discretion and make a gentleperson's agreement not to change instances of it that that the other wrote except when there is a true need to otherwise adjust such code? That seems fine to me. > A reasonable question is how will this be handled by Husht, presuming I can eventually carve out time to move that further? Since we plan for Husht to have Python-like syntax, I think it would make sense to write it in a more Python-like style. The [Python style guide](https://peps.python.org/pep-0008/#indentation) is consistent with the style you're using for long function calls: - First line has the function name and opening parenthesis (no arguments). - Subsequent lines have arguments and are indented one level. - Last line includes both arguments and closing parenthesis. ```rust let sphere = assembly.elements_by_id.with( |elts_by_id| elts_by_id[sphere_id].clone()) ```
glen added 1 commit 2025-11-21 21:09:24 +00:00
chore: relayout per PR discussion/feedback
All checks were successful
/ test (pull_request) Successful in 3m43s
760e811ee4
Author
Owner

@Vectornaut wrote in #128 (comment):

write this particular snippet as

let sphere = assembly.elements_by_id
    .with(|elts_by_id| elts_by_id[sphere_id].clone());

I would certainly prefer it, because it's consistent with the Rust code I'm used to reading.

OK, I have gone through and attempted as much as possible to avoid function call open and close parens on different lines by using other linebreaks, especially the "just before dot" ones. Where linebreaks are required within a function call, I've tried to be more liberal with putting the matching close on its own line. But I do still fall back to they "Pythonesque" layout where I just don't see a good alternative.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3432: > write this particular snippet as > > ```rust > let sphere = assembly.elements_by_id > .with(|elts_by_id| elts_by_id[sphere_id].clone()); > ``` > I would certainly prefer it, because it's consistent with the Rust code I'm used to reading. OK, I have gone through and attempted as much as possible to avoid function call open and close parens on different lines by using other linebreaks, especially the "just before dot" ones. Where linebreaks are required within a function call, I've tried to be more liberal with putting the matching close on its own line. But I do still fall back to they "Pythonesque" layout where I just don't see a good alternative.
Author
Owner

OK, should be good for next round of review.

OK, should be good for next round of review.
Vectornaut requested changes 2025-11-22 08:38:50 +00:00
Vectornaut left a comment
Member

This is looking good! Mostly just editing for consistency at this point. For repetitive comments, if you handle as suggested, you can just indicate with a 👍 and I'll resolve the conversation.

For multiline assignments, I prefer to put the = at the end of the first line, like on line 432 of engine.rs. In the pull request, you've mostly put the = at the beginning of the second line. It would be nice to pick one convention and stick to it.

This is looking good! Mostly just editing for consistency at this point. For repetitive comments, if you handle as suggested, you can just indicate with a 👍 and I'll resolve the conversation. For multiline assignments, I prefer to put the `=` at the end of the first line, like on line 432 of `engine.rs`. In the pull request, you've mostly put the `=` at the beginning of the second line. It would be nice to pick one convention and stick to it.
@ -582,1 +588,3 @@
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
Point::NORM_COMPONENT,
col,
point(x,y,z)[Point::NORM_COMPONENT]
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -929,1 +939,3 @@
console_log!("No velocity to unpack for fresh element \"{}\"", elt.id())
console_log!(
"No velocity to unpack for fresh element \"{}\"",
elt.id()
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -965,1 +976,3 @@
);
let subjects = [0, 1]
.map(|k| Rc::new(Sphere::default(format!("sphere{k}"), k))
as Rc<dyn Element>);
Member

I find this quite hard to read. Would you be willing to consider something like this?

let subjects = [0, 1].map(
    |k| Rc::new(
        Sphere::default(format!("sphere{k}"), k)
    ) as Rc<dyn Element>);
I find this quite hard to read. Would you be willing to consider something like this? ```rust let subjects = [0, 1].map( |k| Rc::new( Sphere::default(format!("sphere{k}"), k) ) as Rc<dyn Element>); ```
Author
Owner

Yes this is a tricky one to split. My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case? Anyhow, I will give it another shot.

Yes this is a tricky one to split. My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case? Anyhow, I will give it another shot.
Member

My current attempt was motivated by your apparent preference to split before a ., rather than just after a (. Is that not the case?

My preferences are:

  1. In languages with C-like syntax, show multi-line nesting using a combination of indentation and paired delimiters, like this:
    foo(
        |u| {
            /* function body */
        },
        [
            /* array contents */
        ],
    );
    
  2. In Rust, following convention, show multi-line chaining using hanging indent and leading ., like this:
    long_variable_name
        .method_call()
        .field_access
    

Since (1) conflicts with your preferences, I asked for (2) in cases where that could prevent the violation of (1).

> My current attempt was motivated by your apparent preference to split before a `.`, rather than just after a `(`. Is that not the case? My preferences are: 1. In languages with C-like syntax, show multi-line nesting using a combination of indentation and paired delimiters, like this: ``` foo( |u| { /* function body */ }, [ /* array contents */ ], ); 2. In Rust, following convention, show multi-line chaining using hanging indent and leading `.`, like this: ``` long_variable_name .method_call() .field_access ``` Since (1) conflicts with your preferences, I asked for (2) in cases where that could prevent the violation of (1).
Member

I find (2) above most readable when more than one dot item is chained, so I guess I'd be fine with switching back to Python-style hanging indent for single method calls. I realize that this is what you had in the previous commit, and I apologize for the flip-flop. I think I'm going to have to give up on finding a compromise position on (1) and just deal with formatting I find difficult to read until we use Husht to switch to a non-C-like syntax.

I find (2) above most readable when more than one dot item is chained, so I guess I'd be fine with switching back to Python-style hanging indent for single method calls. I realize that this is what you had in the previous commit, and I apologize for the flip-flop. I think I'm going to have to give up on finding a compromise position on (1) and just deal with formatting I find difficult to read until we use Husht to switch to a non-C-like syntax.
@ -132,0 +135,4 @@
};
let opacity = if self.ghost.get() {
GHOST_OPACITY
} else { DEFAULT_OPACITY };
Member

I think this is easier to read if either both branches are set off or both branches are inlined:

let opacity = if self.ghost.get() {
    GHOST_OPACITY
} else {
    DEFAULT_OPACITY
};
let opacity =
    if self.ghost.get() { GHOST_OPACITY }
    else { DEFAULT_OPACITY };

The latter option is already used on line 146 of outline.rs.

I think this is easier to read if either both branches are set off or both branches are inlined: ```rust let opacity = if self.ghost.get() { GHOST_OPACITY } else { DEFAULT_OPACITY }; ``` ```rust let opacity = if self.ghost.get() { GHOST_OPACITY } else { DEFAULT_OPACITY }; ``` The latter option is already used on line 146 of `outline.rs`.
Author
Owner

Won't surprise you that I don't see any importance for the two branches of a single if to match in this way, but modulo the location of the = the second is fine so once we figure the = out (always previous line, always next line, or prefer one way but mixed is ok when it helps with layout) i will change this.

Won't surprise you that I don't see any importance for the two branches of a single if to match in this way, but modulo the location of the = the second is fine so once we figure the = out (always previous line, always next line, or prefer one way but mixed is ok when it helps with layout) i will change this.
Vectornaut marked this conversation as resolved
@ -189,1 +197,3 @@
let color = if selected { self.color.map(|channel| 0.2 + 0.8*channel) } else { self.color };
let color = if selected {
self.color.map(|channel| 0.2 + 0.8*channel)
} else { self.color };
Member

I think this is easier to read if either both branches are set off or both branches are inlined, like above on lines 136–138.

I think this is easier to read if either both branches are set off or both branches are inlined, like above on lines 136–138.
@ -608,2 +638,3 @@
|| realization_successful && on_last_step;
if on_manipulable_step && state.selection.with(|sel| sel.len() == 1) {
if on_manipulable_step
&& state.selection.with(|sel| sel.len() == 1)
Member

I think the line beginning with && should be indented four spaces. (Currently, it's only indented three.)

I think the line beginning with `&&` should be indented four spaces. (Currently, it's only indented three.)
Author
Owner

Yes typo (I have not yet bothered to install a rust mode for emacs), will fix

Yes typo (I have not yet bothered to install a rust mode for emacs), will fix
Vectornaut marked this conversation as resolved
@ -653,2 +685,3 @@
if frames_since_last_sample >= SAMPLE_PERIOD {
mean_frame_interval.set((time - last_sample_time) / (SAMPLE_PERIOD as f64));
let SP64 = SAMPLE_PERIOD as f64;
mean_frame_interval.set((time - last_sample_time) / SP64);
Member

The new variable SP64 is confusing to me for various reasons. Since we're taking an extra line anyway, could we do something like this instead?

mean_frame_interval.set(
    (time - last_sample_time) / SAMPLE_PERIOD as f64);
The new variable `SP64` is confusing to me for various reasons. Since we're taking an extra line anyway, could we do something like this instead? ```rust mean_frame_interval.set( (time - last_sample_time) / SAMPLE_PERIOD as f64); ```
Author
Owner

Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Abbreviation is one nice common way of dealing with line breaks. And Rust's conversion/coercion rules/syntax are just truly annoying, so i find the formula much easier to read without internal coercion. What else might we call the local abbreviation for the coercion of the global constant with the very long name?

Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Abbreviation is one nice common way of dealing with line breaks. And Rust's conversion/coercion rules/syntax are just truly annoying, so i find the formula much easier to read without internal coercion. What else might we call the local abbreviation for the coercion of the global constant with the very long name?
Member

Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments.

Oh, I see. Like I said above, I may have to just accept going back to Python-style hanging indent for function calls and deal with the cognitive dissonance until we get to a non-C-like syntax with Husht.

What else might we call the local abbreviation for the coercion of the global constant with the very long name?

I'd use sample_period_f64.

> Well that's how it was, but i was trying to accommodate the preference for breaking at a method call instead of within the arguments. Oh, I see. Like I said [above](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3492), I may have to just accept going back to Python-style hanging indent for function calls and deal with the cognitive dissonance until we get to a non-C-like syntax with Husht. > What else might we call the local abbreviation for the coercion of the global constant with the very long name? I'd use `sample_period_f64`.
Author
Owner

Well, that's too long to avoid the line break, so ok, I will revert to the function call being split across lines.

Well, that's too long to avoid the line break, so ok, I will revert to the function call being split across lines.
Author
Owner

Oh, actually it fits on two lines like this

                    mean_frame_interval
                        .set((time - last_sample_time) / SAMPLE_PERIOD as f64);

which I presume you like better. Don't know why I didn't see that before. So will do that in the next revision.

Oh, actually it fits on two lines like this ``` mean_frame_interval .set((time - last_sample_time) / SAMPLE_PERIOD as f64); ``` which I presume you like better. Don't know why I didn't see that before. So will do that in the next revision.
Vectornaut marked this conversation as resolved
@ -697,0 +728,4 @@
let sphere_reps_world: Vec<_> = scene.spheres.representations
.into_iter().map(
|rep| (&asm_to_world * rep).cast::<f32>()
).collect();
Member

The revised version is pretty close to the one-per-line convention for chained method calls. I'd recommend going all the way there:

let sphere_reps_world: Vec<_> = scene.spheres.representations
    .into_iter()
    .map(|rep| (&asm_to_world * rep).cast::<f32>())
    .collect();
The revised version is pretty close to the one-per-line convention for chained method calls. I'd recommend going all the way there: ```rust let sphere_reps_world: Vec<_> = scene.spheres.representations .into_iter() .map(|rep| (&asm_to_world * rep).cast::<f32>()) .collect(); ```
@ -767,0 +805,4 @@
);
bind_new_buffer_to_attribute(&ctx, point_color_attr,
(COLOR_SIZE + 1) as i32,
scene.points.colors_with_opacity.concat().as_slice());
Member

For consistency, I'd start the arguments on the next line here too.

bind_new_buffer_to_attribute(
    &ctx, point_color_attr,
    (COLOR_SIZE + 1) as i32,
    scene.points.colors_with_opacity.concat().as_slice());
For consistency, I'd start the arguments on the next line here too. ```rust bind_new_buffer_to_attribute( &ctx, point_color_attr, (COLOR_SIZE + 1) as i32, scene.points.colors_with_opacity.concat().as_slice()); ```
Vectornaut marked this conversation as resolved
@ -64,2 +64,4 @@
bind:value = value,
on:change = move |_| {
let specification
= SpecifiedValue::try_from(value.get_clone_untracked());
Member

A SpecifiedValue consists of both a specification (spec) and a value (value), so calling the whole thing specification is confusing to me. I might use specified_value or spec_val.

A `SpecifiedValue` consists of both a specification (`spec`) and a value (`value`), so calling the whole thing `specification` is confusing to me. I might use `specified_value` or `spec_val`.
Author
Owner

Since this is a single use temporary introduced just for the sake of an abbreviation, and its single use is on the very next line, I just called it val.

Since this is a single use temporary introduced just for the sake of an abbreviation, and its single use is on the very next line, I just called it `val`.
Member

To me, turning a variable called value into a variable called spec_val or specified_value seems more understandable than turning a variable called value into a variable called val. In the latter case, the variable names don't make it clear what the point of the processing was. Looking at the line where val is used, one might ask: why didn't we just use value here?

To me, turning a variable called `value` into a variable called `spec_val` or `specified_value` seems more understandable than turning a variable called `value` into a variable called `val`. In the latter case, the variable names don't make it clear what the point of the processing was. Looking at the line where `val` is used, one might ask: why didn't we just use `value` here?
Author
Owner

It's just an abbreviation. It doesn't need much in the way of semantics. temp? x? s? sv? If it wasn't the case that the variable is introduced solely for the sake of linebreaking and is used only once on the very next line, it might be worth this length of naming conversation. This is a throwaway.

It's just an abbreviation. It doesn't need much in the way of semantics. `temp`? `x`? `s`? `sv`? If it wasn't the case that the variable is introduced solely for the sake of linebreaking and is used only once on the very next line, it might be worth this length of naming conversation. This is a throwaway.
Member

It's just an abbreviation.

Oh, I see: you're now trying to find a name short enough to jam that expression into one line. Personally, I think the extra clarity is worth an extra line. However, among the names you proposed just now, sv seems the least confusing to me.

> It's just an abbreviation. Oh, I see: you're now trying to find a name short enough to jam that expression into one line. Personally, I think the extra clarity is worth an extra line. However, among the names you proposed just now, `sv` seems the least confusing to me.
@ -323,1 +334,3 @@
engine::sphere_with_offset(frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, 0.0),
engine::sphere_with_offset(
frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6,
-frac_1_sqrt_6, 0.0
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -329,1 +343,3 @@
engine::sphere_with_offset(-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, 0.0),
engine::sphere_with_offset(
-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6,
-frac_1_sqrt_6, 0.0
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -335,1 +352,3 @@
engine::sphere_with_offset(-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, 0.0),
engine::sphere_with_offset(
-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6,
-frac_1_sqrt_6, 0.0
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -14,0 +12,4 @@
pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)
-> DVector<f64> {
let center_norm_sq
= center_x * center_x + center_y * center_y + center_z * center_z;
Member

I'd match the convention used for sphere_with_offset:

pub fn sphere(
    center_x: f64, center_y: f64, center_z: f64, radius: f64
) -> DVector<f64> {
I'd match the convention used for `sphere_with_offset`: ```rust pub fn sphere( center_x: f64, center_y: f64, center_z: f64, radius: f64 ) -> DVector<f64> { ```
Author
Owner

Here I very much don't see a need for a strict uniform "convention" but rather most readable layout on a case-by-case basis, you said you like function-call open and close on same line when possible, I really universally dislike paren on next line, and of all the places to break functionName(parameter1: Type1, parameter2: Type2) -> ReturnType { just before the arrow seems by far the cleanest to me: before an operator, clean semantic break between the calling convention on the one hand and the return type on the other, arrow makes it nice and clear this is a function. So I would advocate that breakpoint as the very first to use when needed, and only break the parameter list when it simply won't fit on one line.

Thoughts?

Here I very much don't see a need for a strict uniform "convention" but rather most readable layout on a case-by-case basis, you said you like function-call open and close on same line when possible, I really universally dislike paren on next line, and of all the places to break `functionName(parameter1: Type1, parameter2: Type2) -> ReturnType {` just before the arrow seems by far the cleanest to me: before an operator, clean semantic break between the calling convention on the one hand and the return type on the other, arrow makes it nice and clear this is a function. So I would advocate that breakpoint as the very first to use when needed, and only break the parameter list when it simply won't fit on one line. Thoughts?
Member

The main thing I find confusing about the current formatting is that the -> DVector<f64> is part of the function declaration, but no indentation is used to show this. If you find bracket on next line more palatable than parenthesis on next line, I'd find something like this more readable than the current formatting:

pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)
    -> DVector<f64>
{
The main thing I find confusing about the current formatting is that the `-> DVector<f64>` is part of the function declaration, but no indentation is used to show this. If you find bracket on next line more palatable than parenthesis on next line, I'd find something like this more readable than the current formatting: ```rust pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVector<f64> { ```
Author
Owner

I'll admit I am looking for a two-line solution here. I don't see any reason to indent the -> just as there is no reason to
indent the else of an if -- to me, the -> reads as a companion keyword to fn. So for the upcoming revision, I will try moving just the parenthesis to the next line to see if you like that better. As little as I like a linebreak before a paren, I like wasting lines less ;-)

I'll admit I am looking for a two-line solution here. I don't see any reason to indent the `->` just as there is no reason to indent the `else` of an `if` -- to me, the `->` reads as a companion keyword to `fn`. So for the upcoming revision, I will try moving just the parenthesis to the next line to see if you like that better. As little as I like a linebreak before a paren, I like wasting lines less ;-)
Member

I'll admit I am looking for a two-line solution here.

All of the reasonable two-line solutions I can think of seem equally hard to read to me, and equally in violation of the Rust and Python style guides. Here's one last suggestion, which is inspired by (but in violation of) the Python style guide. You're welcome to choose between this formatting and the current one.

pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64)
        -> DVector<f64> {
    /* function body */
> I'll admit I am looking for a two-line solution here. All of the reasonable two-line solutions I can think of seem equally hard to read to me, and equally in violation of the [Rust](https://doc.rust-lang.org/beta/style-guide/index.html) and [Python](https://peps.python.org/pep-0008/#indentation) style guides. Here's one last suggestion, which is inspired by (but in violation of) the Python style guide. You're welcome to choose between this formatting and the current one. ```rust pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) -> DVector<f64> { /* function body */ ```
Author
Owner

Well, from my point of view we discarded double-indent patterns for if and for so I am loath to revive them here, so I think we are settling on

pub fn foo(long: stuff
) -> ReturnType {
    // body
}

Please confirm, thanks.

Well, from my point of view we discarded double-indent patterns for `if` and `for` so I am loath to revive them here, so I think we are settling on ``` pub fn foo(long: stuff ) -> ReturnType { // body } ``` Please confirm, thanks.
Member

Please confirm, thanks.

Confirmed.

> Please confirm, thanks. Confirmed.
Vectornaut marked this conversation as resolved
@ -202,2 +204,3 @@
// projection inner product
pub fn proj(&self, v: &DVectorView<f64>, column_index: usize) -> DMatrix<f64> {
pub fn proj(&self, v: &DVectorView<f64>, column_index: usize)
-> DMatrix<f64> {
Member

I'd match the convention used for sphere_with_offset. It adds one line, but for me the gain in readability and familiarity are worth it.

pub fn proj(
    &self, v: &DVectorView<f64>, column_index: usize
) -> DMatrix<f64> {
I'd match the convention used for `sphere_with_offset`. It adds one line, but for me the gain in readability and familiarity are worth it. ```rust pub fn proj( &self, v: &DVectorView<f64>, column_index: usize ) -> DMatrix<f64> { ```
Vectornaut marked this conversation as resolved
@ -294,1 +297,3 @@
fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize) -> DMatrix<f64> {
fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize)
-> DMatrix<f64>
{
Member

I'd stick to the convention used for sphere_with_offset:

fn basis_matrix(
    index: (usize, usize), nrows: usize, ncols: usize
) -> DMatrix<f64> {
I'd stick to the convention used for `sphere_with_offset`: ```rust fn basis_matrix( index: (usize, usize), nrows: usize, ncols: usize ) -> DMatrix<f64> { ```
Vectornaut marked this conversation as resolved
@ -612,0 +624,4 @@
problem.gram.push_sym(
block + j,
block + k,
if j == k { 0.0 } else { -0.5 }
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -705,1 +722,3 @@
problem.gram.push_sym(j, k, if (j, k) == (1, 1) { 1.0 } else { 0.0 });
problem.gram.push_sym(
j, k,
if (j, k) == (1, 1) { 1.0 } else { 0.0 }
Member

In our current convention, this argument list needs a trailing comma on the last line.

In our current convention, this argument list needs a trailing comma on the last line.
Vectornaut marked this conversation as resolved
@ -805,2 +831,3 @@
fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize) -> Vec<DVector<f64>> {
fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize)
-> Vec<DVector<f64>> {
Member

I'd stick to the convention used for sphere_with_offset. It adds one line, but for me the gain in readability and familiarity are worth it.

fn translation_motion_unif(
    vel: &Vector3<f64>, assembly_dim: usize
) -> Vec<DVector<f64>> {
I'd stick to the convention used for `sphere_with_offset`. It adds one line, but for me the gain in readability and familiarity are worth it. ```rust fn translation_motion_unif( vel: &Vector3<f64>, assembly_dim: usize ) -> Vec<DVector<f64>> { ```
Vectornaut marked this conversation as resolved
Author
Owner

@Vectornaut wrote in #128 (comment):

. In the pull request, you've mostly put the = at the beginning of the second line. It would be nice to pick one convention and stick to it.

Well, as you know. I am much less concerned with strict uniformity in such things since (as this PR shows) it's a pain to achieve and i just don't see benefits from it that pay for that pain.

I am also a strong advocate for laying out code as closely as one would lay out other forms of text, equations in articles, etc. For example, that's one of the reasons i really dislike parenthesis at the beginning of a line -- there is absolutely no other form of writing in which you would ever do that. If i were setting hard-and-fast layout rules for our code, they would include never doing this. But in the spirit of consensus, i have tried to use them in places that i think you would find them most called for.

So, on this point, if you websearch "equals at end of one line or beginning of next" you will find overwhelming preponderance of recommendations for beginning of next, in line with the general recommendation of splitting just before operators rather than just after.

So if we are going to attempt complete uniformity in this, I would recommend changing any remaining trailing '=' to the beginning of the next line.

Thoughts?

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3475: > . In the pull request, you've mostly put the `=` at the beginning of the second line. It would be nice to pick one convention and stick to it. Well, as you know. I am much less concerned with strict uniformity in such things since (as this PR shows) it's a pain to achieve and i just don't see benefits from it that pay for that pain. I am also a strong advocate for laying out code as closely as one would lay out other forms of text, equations in articles, etc. For example, that's one of the reasons i really dislike parenthesis at the beginning of a line -- there is absolutely no other form of writing in which you would ever do that. If i were setting hard-and-fast layout rules for our code, they would include never doing this. But in the spirit of consensus, i have tried to use them in places that i think you would find them most called for. So, on this point, if you websearch "equals at end of one line or beginning of next" you will find overwhelming preponderance of recommendations for beginning of next, in line with the general recommendation of splitting just before operators rather than just after. So if we are going to attempt complete uniformity in this, I would recommend changing any remaining trailing '=' to the beginning of the next line. Thoughts?
Author
Owner

I won't be able to revise the PR again until Monday at soonest, but I took a little tike this morning to reply on points you can consider in the meantime. Thanks!

I won't be able to revise the PR again until Monday at soonest, but I took a little tike this morning to reply on points you can consider in the meantime. Thanks!
Author
Owner

@Vectornaut wrote in #128 (comment):

For repetitive comments, if you handle as suggested, you can just indicate with a 👍

Or actually once I do push a commit, you can assume I just took any suggestion i didn't comment on (or comment on a twin of).

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3475: > For repetitive comments, if you handle as suggested, you can just indicate with a :+1: Or actually once I do push a commit, you can assume I just took any suggestion i didn't comment on (or comment on a twin of).
Member

I am also a strong advocate for laying out code as closely as one would lay out other forms of text, equations in articles, etc.

This is helpful guideline, and we should keep it in mind when thinking about syntax and style conventions for Husht.

So, on this point, if you websearch "equals at end of one line or beginning of next" you will find overwhelming preponderance of recommendations for beginning of next, in line with the general recommendation of splitting just before operators rather than just after.

A DuckDuckGo search doesn't return many relevant results for me, but the top result is for mathematical equations, where = is typically used for equality rather than assignment, and in particular is often chained:

long expression
  = longer expression
  = another long expression
  = some expression

When equality is used for assignment, I think that putting the = at the end of the first line makes it easier to format lists of terms, as I do here:

let neg_d_err =
    basis_mat.tr_mul(&*Q) * &state.config
    + state.config.tr_mul(&*Q) * &basis_mat;

So if we are going to attempt complete uniformity in this, I would recommend changing any remaining trailing '=' to the beginning of the next line.

As long as you have a readable way to format the example above, this works for me.

> I am also a strong advocate for laying out code as closely as one would lay out other forms of text, equations in articles, etc. This is helpful guideline, and we should keep it in mind when thinking about syntax and style conventions for Husht. > So, on this point, if you websearch "equals at end of one line or beginning of next" you will find overwhelming preponderance of recommendations for beginning of next, in line with the general recommendation of splitting just before operators rather than just after. A DuckDuckGo search doesn't return many relevant results for me, but the [top result](https://math.stackexchange.com/questions/4220839/correct-position-of-equal-sign-in-multiline-equation) is for mathematical equations, where `=` is typically used for equality rather than assignment, and in particular is often chained: ``` long expression = longer expression = another long expression = some expression ``` When equality is used for assignment, I think that putting the `=` at the end of the first line makes it easier to format lists of terms, as I do [here](https://code.studioinfinity.org/StudioInfinity/dyna3/src/commit/6c3a48fb52a02f33ff2823b69357d2d5f44efe94/app-proto/src/engine.rs#L426-L428): ```rust let neg_d_err = basis_mat.tr_mul(&*Q) * &state.config + state.config.tr_mul(&*Q) * &basis_mat; ``` > So if we are going to attempt complete uniformity in this, I would recommend changing any remaining trailing '=' to the beginning of the next line. As long as you have a readable way to format the example above, this works for me.
Member

I think the main obstacle here is my preferred way to format nesting in language with C-like syntax, described above as preference (1). Preference (1) is used throughout the codebase, and it's incompatible with your formatting preferences, so this pull request will introduce formatting inconsistencies no matter how much we negotiate.

At this point, I'd be willing to simply accept the indentation conventions of the Python style guide for lines that need to be wrapped, since it seems likely that we'll eventually adopt a similar convention for Husht.

I think the main obstacle here is my preferred way to format nesting in language with C-like syntax, [described above](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3491) as preference (1). Preference (1) is used throughout the codebase, and it's incompatible with your formatting preferences, so this pull request will introduce formatting inconsistencies no matter how much we negotiate. At this point, I'd be willing to simply accept the indentation conventions of the [Python style guide](https://peps.python.org/pep-0008/#indentation) for lines that need to be wrapped, since it seems likely that we'll eventually adopt a similar convention for Husht.
Author
Owner

@Vectornaut wrote in #128 (comment):

as I do here:

let neg_d_err =
    basis_mat.tr_mul(&*Q) * &state.config
    + state.config.tr_mul(&*Q) * &basis_mat;

As long as you have a readable way to format the example above, this works for me.

Well, there are no lines over 80 characters here, so I would be fine simply leaving this code alone for now until there is some specific need to revisit it. If you feel that we need to get all declarations/assigments in one consistent format in this PR, then OK, we can extend this PR from just wrapping long lines, to that and making = formatting consistent.

But personally I would format this as

let neg_d_err = basis_mat.tr_mul(&*Q) * &state.config
    + state.config.tr_mul(&*Q) * &basis_mat;

so there is no issue of where the = goes. That issue only comes up when the best linebreak within the first 80 characters happens to be at the = sign, which I don't see being the case here.

Just to examine the contours of this problem, I am guessing you like the first option below better.

[
    A,
    B,
    C,
]

vs

[A,
    B,
    C,
]

For me, the first form is just wasting a line (well, actually 2, but I wanted to leave the final bracket out of this for now).

So my approach in breaking an assignment is to just start the assignment, and if it runs past 80 characters, go back and see if there's a natural place to break it so that it will fit on just two lines total. That place may be at the =, but that's fairly low on my preference list. And if it is there, then my preference is to break just before the = rather than just after, for the exact same reason we put + or * at the beginning of the next line -- because it's easier to see there when one is just scanning some code. This end-of-previous vs beginning-of-next issue is actually more important in rust, because the thing being assigned to may be an intricate and hence quite long destructuring pattern. So I think it is very helpful to have the = out in front as a sort of separator between the specifier (of where the assignment will go) and the value (that will go into the location described by the specifier).

Not sure whether we are converging here, or how to do so if not.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3495: > as I do [here](https://code.studioinfinity.org/StudioInfinity/dyna3/src/commit/6c3a48fb52a02f33ff2823b69357d2d5f44efe94/app-proto/src/engine.rs#L426-L428): > > ```rust > let neg_d_err = > basis_mat.tr_mul(&*Q) * &state.config > + state.config.tr_mul(&*Q) * &basis_mat; > ``` > As long as you have a readable way to format the example above, this works for me. Well, there are no lines over 80 characters here, so I would be fine simply leaving this code alone for now until there is some specific need to revisit it. If you feel that we need to get all declarations/assigments in one consistent format in this PR, then OK, we can extend this PR from just wrapping long lines, to that _and_ making `=` formatting consistent. But personally I would format this as ``` let neg_d_err = basis_mat.tr_mul(&*Q) * &state.config + state.config.tr_mul(&*Q) * &basis_mat; ``` so there is no issue of where the `=` goes. That issue only comes up when the best linebreak within the first 80 characters happens to be at the `=` sign, which I don't see being the case here. Just to examine the contours of this problem, I am guessing you like the first option below better. ``` [ A, B, C, ] ``` vs ``` [A, B, C, ] ``` For me, the first form is just wasting a line (well, actually 2, but I wanted to leave the final bracket out of this for now). So my approach in breaking an assignment is to just start the assignment, and if it runs past 80 characters, go back and see if there's a natural place to break it so that it will fit on just two lines total. That place may be at the =, but that's fairly low on my preference list. And if it is there, then my preference is to break just before the = rather than just after, for the exact same reason we put + or * at the beginning of the next line -- because it's easier to see there when one is just scanning some code. This end-of-previous vs beginning-of-next issue is actually more important in rust, because the thing being assigned _to_ may be an intricate and hence quite long destructuring pattern. So I think it is very helpful to have the `=` out in front as a sort of separator between the specifier (of where the assignment will go) and the value (that will go into the location described by the specifier). Not sure whether we are converging here, or how to do so if not.
glen added 1 commit 2025-11-25 00:31:27 +00:00
chore: further reformatting per review
All checks were successful
/ test (pull_request) Successful in 3m42s
e71b0944f6
Vectornaut requested changes 2025-11-27 00:12:23 +00:00
Vectornaut left a comment
Member

Looks like there's just two small tweaks left, including the temporary variable naming decision.

Looks like there's just two small tweaks left, including the [temporary variable naming decision](https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/128#issuecomment-3509).
@ -582,1 +588,3 @@
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
Point::NORM_COMPONENT,
col,
point(x,y,z)[Point::NORM_COMPONENT],
Member

Our current convention is to put spaces after the commas between arguments.

Our current convention is to put spaces after the commas between arguments.
All checks were successful
/ test (pull_request) Successful in 3m42s
Required
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u aroundLineInEightyChars:glen-aroundLineInEightyChars
git checkout glen-aroundLineInEightyChars
Sign in to join this conversation.
No description provided.