chore: wrap at 80 characters #128

Open
glen wants to merge 3 commits from glen/dyna3:aroundLineInEightyChars into main
5 changed files with 36 additions and 42 deletions
Showing only changes of commit e71b0944f6 - Show all commits

View file

@ -587,7 +587,7 @@ impl ProblemPoser for PointCoordinateRegulator {
problem.frozen.push( problem.frozen.push(
Point::NORM_COMPONENT, Point::NORM_COMPONENT,
col, col,
point(x,y,z)[Point::NORM_COMPONENT] point(x,y,z)[Point::NORM_COMPONENT],
Vectornaut marked this conversation as resolved Outdated

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.

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

Our current convention is to put spaces after the commas between arguments.
); );
} }
} }
@ -938,7 +938,7 @@ impl Assembly {
None => { None => {
console_log!( console_log!(
"No velocity to unpack for fresh element \"{}\"", "No velocity to unpack for fresh element \"{}\"",
elt.id() elt.id(),
Vectornaut marked this conversation as resolved Outdated

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.
) )
}, },
}; };
@ -973,9 +973,9 @@ mod tests {
inversive distance regulator writes problem data")] inversive distance regulator writes problem data")]
fn unindexed_subject_test_inversive_distance() { fn unindexed_subject_test_inversive_distance() {
let _ = create_root(|| { let _ = create_root(|| {
let subjects = [0, 1] let subjects = [0, 1].map(
.map(|k| Rc::new(Sphere::default(format!("sphere{k}"), k)) |k| Rc::new(Sphere::default(format!("sphere{k}"), k)) as Rc<dyn Element>
as Rc<dyn Element>); );

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>); ```

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.

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).

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.
subjects[0].set_column_index(0); subjects[0].set_column_index(0);
InversiveDistanceRegulator { InversiveDistanceRegulator {
subjects: subjects, subjects: subjects,

View file

@ -128,14 +128,10 @@ impl DisplayItem for Sphere {
const HIGHLIGHT: f32 = 0.2; const HIGHLIGHT: f32 = 0.2;
let representation = self.representation.get_clone_untracked(); let representation = self.representation.get_clone_untracked();
let color = if selected { let color = if selected { self.color.map(|channel| 0.2 + 0.8*channel) }
self.color.map(|channel| 0.2 + 0.8*channel) else { self.color };
} else { let opacity = if self.ghost.get() { GHOST_OPACITY }
self.color else { DEFAULT_OPACITY };
};
let opacity = if self.ghost.get() {
GHOST_OPACITY
} else { DEFAULT_OPACITY };
let highlight = if selected { 1.0 } else { HIGHLIGHT }; let highlight = if selected { 1.0 } else { HIGHLIGHT };
scene.spheres.push(representation, color, opacity, highlight); scene.spheres.push(representation, color, opacity, highlight);
} }
@ -194,9 +190,8 @@ impl DisplayItem for Point {
const HIGHLIGHT: f32 = 0.5; const HIGHLIGHT: f32 = 0.5;
let representation = self.representation.get_clone_untracked(); let representation = self.representation.get_clone_untracked();
let color = if selected { let color = if selected { self.color.map(|channel| 0.2 + 0.8*channel) }
self.color.map(|channel| 0.2 + 0.8*channel) else { self.color };
} else { self.color };
let opacity = if self.ghost.get() { GHOST_OPACITY } else { 1.0 }; let opacity = if self.ghost.get() { GHOST_OPACITY } else { 1.0 };
let highlight = if selected { 1.0 } else { HIGHLIGHT }; let highlight = if selected { 1.0 } else { HIGHLIGHT };
scene.points.push(representation, color, opacity, highlight, selected); scene.points.push(representation, color, opacity, highlight, selected);
@ -637,7 +632,7 @@ pub fn Display() -> View {
!realization_successful && on_init_step !realization_successful && on_init_step
|| realization_successful && on_last_step; || realization_successful && on_last_step;
if on_manipulable_step if on_manipulable_step
&& state.selection.with(|sel| sel.len() == 1) && state.selection.with(|sel| sel.len() == 1)
{ {
let sel = state.selection.with( let sel = state.selection.with(
|sel| sel.into_iter().next().unwrap().clone() |sel| sel.into_iter().next().unwrap().clone()
@ -683,8 +678,8 @@ pub fn Display() -> View {
// measure mean frame interval // measure mean frame interval
frames_since_last_sample += 1; frames_since_last_sample += 1;
if frames_since_last_sample >= SAMPLE_PERIOD { if frames_since_last_sample >= SAMPLE_PERIOD {
let SP64 = SAMPLE_PERIOD as f64; mean_frame_interval
mean_frame_interval.set((time - last_sample_time) / SP64); .set((time - last_sample_time) / SAMPLE_PERIOD as f64);
last_sample_time = time; last_sample_time = time;
frames_since_last_sample = 0; frames_since_last_sample = 0;
} }
@ -726,9 +721,9 @@ pub fn Display() -> View {
// write the spheres in world coordinates // write the spheres in world coordinates
let sphere_reps_world: Vec<_> = scene.spheres.representations let sphere_reps_world: Vec<_> = scene.spheres.representations
.into_iter().map( .into_iter()
|rep| (&asm_to_world * rep).cast::<f32>() .map(|rep| (&asm_to_world * rep).cast::<f32>())
).collect(); .collect();
// set the resolution // set the resolution
let width = canvas.width() as f32; let width = canvas.width() as f32;
@ -803,7 +798,8 @@ pub fn Display() -> View {
&ctx, point_position_attr, &ctx, point_position_attr,
SPACE_DIM as i32, point_positions.as_slice(), SPACE_DIM as i32, point_positions.as_slice(),
); );
bind_new_buffer_to_attribute(&ctx, point_color_attr, bind_new_buffer_to_attribute(
&ctx, point_color_attr,
(COLOR_SIZE + 1) as i32, (COLOR_SIZE + 1) as i32,
scene.points.colors_with_opacity.concat().as_slice()); scene.points.colors_with_opacity.concat().as_slice());
bind_new_buffer_to_attribute( bind_new_buffer_to_attribute(
@ -969,9 +965,9 @@ pub fn Display() -> View {
.into_iter() .into_iter()
.filter(|elt| !elt.ghost().get()); .filter(|elt| !elt.ghost().get());
for elt in tangible_elts { for elt in tangible_elts {
let hit = assembly_to_world.with( let cast = assembly_to_world.with(
|asm_to_world| elt.cast(dir, asm_to_world, pixel_size)); |asm_to_world| elt.cast(dir, asm_to_world, pixel_size));
match hit { match cast {
Some(depth) => match clicked { Some(depth) => match clicked {
Some((_, best_depth)) => { Some((_, best_depth)) => {
if depth < best_depth { if depth < best_depth {

View file

@ -63,10 +63,9 @@ fn RegulatorInput(regulator: Rc<dyn Regulator>) -> View {
placeholder = measurement.with(|result| result.to_string()), placeholder = measurement.with(|result| result.to_string()),
bind:value = value, bind:value = value,
on:change = move |_| { on:change = move |_| {
let specification let val = SpecifiedValue::try_from(value.get_clone_untracked());
= SpecifiedValue::try_from(value.get_clone_untracked());
valid.set( valid.set(

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`.

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`.

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?

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.

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.
match specification { match val {
Ok(set_pt) => { Ok(set_pt) => {
set_point.set(set_pt); set_point.set(set_pt);
true true

View file

@ -333,7 +333,7 @@ fn load_tridiminished_icosahedron(assembly: &Assembly) {
COLOR_FACE, COLOR_FACE,
engine::sphere_with_offset( engine::sphere_with_offset(
frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6,
-frac_1_sqrt_6, 0.0 -frac_1_sqrt_6, 0.0,
Vectornaut marked this conversation as resolved Outdated

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.
), ),
), ),
Sphere::new( Sphere::new(
@ -342,7 +342,7 @@ fn load_tridiminished_icosahedron(assembly: &Assembly) {
COLOR_FACE, COLOR_FACE,
engine::sphere_with_offset( engine::sphere_with_offset(
-frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6,
-frac_1_sqrt_6, 0.0 -frac_1_sqrt_6, 0.0,
Vectornaut marked this conversation as resolved Outdated

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.
), ),
), ),
Sphere::new( Sphere::new(
@ -351,7 +351,7 @@ fn load_tridiminished_icosahedron(assembly: &Assembly) {
COLOR_FACE, COLOR_FACE,
engine::sphere_with_offset( engine::sphere_with_offset(
-frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6, -frac_1_sqrt_6, -frac_1_sqrt_6, frac_2_sqrt_6,
-frac_1_sqrt_6, 0.0 -frac_1_sqrt_6, 0.0,
Vectornaut marked this conversation as resolved Outdated

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.
), ),
), ),
]; ];

View file

@ -9,8 +9,8 @@ pub fn point(x: f64, y: f64, z: f64) -> DVector<f64> {
} }
// the sphere with the given center and radius, with inward-pointing normals // the sphere with the given center and radius, with inward-pointing normals
pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64) pub fn sphere(center_x: f64, center_y: f64, center_z: f64, radius: f64
-> DVector<f64> { ) -> DVector<f64> {
let center_norm_sq let center_norm_sq
= center_x * center_x + center_y * center_y + center_z * center_z; = center_x * center_x + center_y * center_y + center_z * center_z;
Vectornaut marked this conversation as resolved

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> { ```

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?

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> { ```

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 ;-)

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 */ ```

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.

Please confirm, thanks.

Confirmed.

> Please confirm, thanks. Confirmed.
DVector::from_column_slice(&[ DVector::from_column_slice(&[
@ -202,8 +202,8 @@ impl ConfigSubspace {
// with the given column index has velocity `v`. the velocity is given in // with the given column index has velocity `v`. the velocity is given in
// projection coordinates, and the projection is done with respect to the // projection coordinates, and the projection is done with respect to the
// projection inner product // projection inner product
pub fn proj(&self, v: &DVectorView<f64>, column_index: usize) pub fn proj(&self, v: &DVectorView<f64>, column_index: usize
-> DMatrix<f64> { ) -> DMatrix<f64> {
Vectornaut marked this conversation as resolved Outdated

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> { ```
if self.dim() == 0 { if self.dim() == 0 {
const ELEMENT_DIM: usize = 5; const ELEMENT_DIM: usize = 5;
DMatrix::zeros(ELEMENT_DIM, self.assembly_dim) DMatrix::zeros(ELEMENT_DIM, self.assembly_dim)
@ -294,9 +294,8 @@ impl SearchState {
} }
} }
fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize) fn basis_matrix(index: (usize, usize), nrows: usize, ncols: usize
-> DMatrix<f64> ) -> DMatrix<f64> {
{
let mut result = DMatrix::<f64>::zeros(nrows, ncols); let mut result = DMatrix::<f64>::zeros(nrows, ncols);
Vectornaut marked this conversation as resolved Outdated

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> { ```
result[index] = 1.0; result[index] = 1.0;
result result
@ -624,7 +623,7 @@ pub mod examples {
problem.gram.push_sym( problem.gram.push_sym(
block + j, block + j,
block + k, block + k,
if j == k { 0.0 } else { -0.5 } if j == k { 0.0 } else { -0.5 },
); );
Vectornaut marked this conversation as resolved

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.
} }
@ -721,7 +720,7 @@ mod tests {
for k in j..2 { for k in j..2 {
problem.gram.push_sym( problem.gram.push_sym(
j, k, j, k,
if (j, k) == (1, 1) { 1.0 } else { 0.0 } if (j, k) == (1, 1) { 1.0 } else { 0.0 },
); );
Vectornaut marked this conversation as resolved

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.
} }
} }
@ -829,8 +828,8 @@ mod tests {
} }
} }
fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize) fn translation_motion_unif(vel: &Vector3<f64>, assembly_dim: usize
-> Vec<DVector<f64>> { ) -> Vec<DVector<f64>> {
let mut elt_motion = DVector::zeros(4); let mut elt_motion = DVector::zeros(4);
Vectornaut marked this conversation as resolved Outdated

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>> { ```
elt_motion.fixed_rows_mut::<3>(0).copy_from(vel); elt_motion.fixed_rows_mut::<3>(0).copy_from(vel);
iter::repeat(elt_motion).take(assembly_dim).collect() iter::repeat(elt_motion).take(assembly_dim).collect()