Skip to content
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

add whitespace in assert!s in core::char #101231

Closed

Conversation

oppiliappan
Copy link
Contributor

when assert!-ing an expression prefixed with !, it moves the
relevant expression by 1 character. to make such expressions contrast
with assert!ions on expressions that are not prefixed with !, i
suggest adding a space. this is in alignment with the docs for
std::ops::Range.


i was not able to run ./x.py doc library or ./x.py test tidy, i got the following error on NixOS:

error: /nix/store/jsp3h3wpzc842j0rz61m5ly71ak6qgdn-glibc-2.32-54/lib/libc.so.6: version `GLIBC_2.33' not found (required by /home/np/code/rust/contrib/rust-compiler/build/bootstrap/debug/deps/libserde_derive-a3bb7fde49d5a0ce.so)

which is strange because:

$ ldd --version
ldd (GNU libc) 2.35
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Written by Roland McGrath and Ulrich Drepper.
full logs here
λ ./x.py doc library
Building rustbuild
   Compiling serde v1.0.137
error: /nix/store/jsp3h3wpzc842j0rz61m5ly71ak6qgdn-glibc-2.32-54/lib/libc.so.6: version `GLIBC_2.33' not found (required by /home/np/code/rust/contrib/rust-compiler/build/bootstrap/debug/deps/libserde_derive-a3bb7fde49d5a0ce.so)
   --> /home/np/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.137/src/lib.rs:292:1
    |
292 | extern crate serde_derive;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0432]: unresolved imports `self::__private`, `self::__private`
   --> /home/np/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.137/src/lib.rs:274:5
    |
274 | use self::__private as export;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
275 | #[allow(unused_imports)]
276 | use self::__private as private;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0432`.
error: could not compile `serde` due to 2 previous errors
failed to run: /home/np/code/rust/contrib/rust-compiler/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /home/np/code/rust/contrib/rust-compiler/src/bootstrap/Cargo.toml
Build completed unsuccessfully in 0:00:02

any help on that front would be great, unless it is not required for this PR.

when `assert!`-ing an expression prefixed with `!`, it moves the
relevant expression by 1 character. to make such expressions contrast
with `assert!`ions on expressions that are not prefixed with `!`, i
suggest adding a space. this is in alignment with the docs for
`std::ops::Range`.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 31, 2022
@rust-highfive
Copy link
Collaborator

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2022
@thomcc
Copy link
Member

thomcc commented Aug 31, 2022

I don't think this is a desirable change. We don't perform this sort of code alignment anywhere else in the code, and this does not match up with what rustfmt does the way we have it configured (I don't even think it has a setting for this).

I suppose this could be something the proposed style team could decide on, but it goes against existing Rust convention (as enforced by rustfmt, anyway).

@thomcc
Copy link
Member

thomcc commented Aug 31, 2022

Regarding the issue you have with x.py on NixOS, I'm out of my depth there. You should file a separate issue about that, since I believe it should be supported.

@oppiliappan
Copy link
Contributor Author

I don't think this is a desirable change. We don't perform this sort of code alignment anywhere else in the code, and this does not match up with what rustfmt does the way we have it configured (I don't even think it has a setting for this).

as mentioned in the commit message, this spacing is present in the docs for std::ops::Range, for example: Range::contains:

assert!(!(3..5).contains(&2));
assert!( (3..5).contains(&3));
assert!( (3..5).contains(&4));
assert!(!(3..5).contains(&5));

Regarding the issue you have with x.py on NixOS, I'm out of my depth there. You should file a separate issue about that, since I believe it should be supported.

no worries, thanks for the pointer!

@joshtriplett
Copy link
Member

I can confirm that this kind of visual-alignment is not the standard Rust style. If you observe that style in documentation, please feel free to send a patch reformatting that code using rustfmt.

@oppiliappan
Copy link
Contributor Author

@joshtriplett personally, i find it a lot easier to parse and understand the docs when written in the style it has been written in for Range, but I understand if maintaining conventions and uniformity is of greater importance.

@joshtriplett
Copy link
Member

@nerdypepper I agree; this is a case where consistency and case-by-case formatting suggest different answers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants