Skip to content

Add rustfmt check to CI #922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

jturner314
Copy link
Member

@jturner314 jturner314 commented Feb 18, 2021

rustfmt is very convenient when writing PRs, but when it's not enforced by CI, the formatting in the project inevitably diverges from the rustfmt style. So, when I run rustfmt to format a PR, lines unrelated to the changes in the PR are reformatted (since those lines don't follow the rustfmt style), and then I have to manually stage only the relevant lines and discard the irrelevant formatting changes. If rustfmt style is enforced by CI, then I can just run rustfmt before submitting a PR and not have to worry about discarding unrelated formatting changes (since there shouldn't be any).

This PR should fail CI right now. Once we've merged the large open PRs, we can run cargo fmt on the whole crate, rerun CI for this PR to make sure it passes, and then merge this PR.

@bluss bluss self-requested a review February 18, 2021 18:31
@bluss
Copy link
Member

bluss commented Feb 20, 2021

While we certainly need some formatting help sometimes, I think rustfmt makes the wrong decisions too often.

Many good suggestions all in all but the below are not that IMO.

 
-pub(crate) fn can_index_slice_not_custom<D: Dimension>(data_len: usize, dim: &D)
-    -> Result<(), ShapeError>
-{
+pub(crate) fn can_index_slice_not_custom<D: Dimension>(
+    data_len: usize,
+    dim: &D,
+) -> Result<(), ShapeError> {
     // Condition 1.
-    #[deprecated(note = "This method is hard to use correctly. Use `uninit` instead.",
-                 since = "0.15.0")]
+    #[deprecated(
+        note = "This method is hard to use correctly. Use `uninit` instead.",
+        since = "0.15.0"
+    )]
     /// Create an array with uninitalized elements, shape `shape`.
          }
-        panic!("swap: index out of bounds for indices {:?} {:?}", index1, index2);
+        panic!(
+            "swap: index out of bounds for indices {:?} {:?}",
+            index1, index2
+        );
     }
                  let strides = unlimited_transmute::<D, D2>(self.strides);
-                return Ok(ArrayBase::from_data_ptr(self.data, self.ptr)
-                            .with_strides_dim(strides, dim));
-            } else if D::NDIM == None || D2::NDIM == None { // one is dynamic dim
+                return Ok(
+                    ArrayBase::from_data_ptr(self.data, self.ptr).with_strides_dim(strides, dim)
+                );
+            } else if D::NDIM == None || D2::NDIM == None {
+                // one is dynamic dim
                 // safe because dim, strides are equivalent under a different type
                 if let Some(dim) = D2::from_dimension(&self.dim) {

All examples are a bit of the same theme - so maybe it can be tuned

@jturner314
Copy link
Member Author

That makes sense. I prioritize the usefulness of automatic formatting over the quality of the formatting, but that's just my personal preference.

The various rustfmt configuration options are listed here. I don't see any way to configure rustfmt to preserve the formatting in the first example. For the second example, there are options to preserve this indentation style (called "Visual" in the configuration) for some things (imports, expressions, function args, etc.), but I don't see an existing option like this for attributes. The third example could work by increasing the relevant width option. The fourth example may work with overflow_delimited_expr set to true, but I'm not sure.

@bluss bluss mentioned this pull request Apr 7, 2021
@bluss bluss mentioned this pull request Apr 17, 2021
@dvc94ch
Copy link

dvc94ch commented Apr 21, 2021

My two cents are that the first three examples aren't that bad, but might take some getting used to. The last one isn't very good, but the way to fix it would be to split it into two lines like this:

let base = ArrayBase::from_data_ptr(self.data, self.ptr);
Ok(base.with_strides_dim(strides, dim))

@bluss
Copy link
Member

bluss commented Apr 22, 2021

I think the way to go is to use unstable format settings, as soon as rustfmt 2 is out it is also possible on stable.

As long as I can fix example 1 I'm ready to go.

@danieleades
Copy link

I strongly believe that the value of consistent formatting massively outweighs the pain of small disagreements about the formatting of specific cases.

That said, if you can find a configuration that does what you want, everyone wins

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants