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

Readability improvements #4928

Closed
wants to merge 5 commits into from
Closed

Conversation

robertbastian
Copy link
Member

This started out as docs improvements, but I've also touched non-docs code now.

let collator: Collator = Collator::try_new(...);

becomes

let collator = Collator::try_new(...);

======

let asdf = foo.iter().map(...).collect::<Vec<_>>();

becomes

let asdf = Vec::from_iter(foo.iter().map(...));

(this shorter, less symbol salad, does into_iter(), and puts the type upfront)

======

let asdf: ZeroMap<u8, u8> = ZeroMap::from_foo();

becomes

let asdf = ZeroMap::<u8, u8>::from_foo();

(this is shorter, and better for refactorings, as the expression has a type)

======

let loc: Locale = "foo".parse().unwrap();

becomes

let loc = "foo".parse::<Locale>().unwrap();

(this is better for refactorings, as the expression has a type)

@sffc
Copy link
Member

sffc commented May 23, 2024

I think I prefer foo.iter().map(...).collect() because directly using from_iter doesn't chain, which negatively impacts indentation. The rest seem fine.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, CI is failing.

@robertbastian
Copy link
Member Author

I think I prefer foo.iter().map(...).collect() because directly using from_iter doesn't chain,

I didn't touch collects without a turbofish.

This often reduces line breaking, many expressions become one-liners.

@robertbastian robertbastian requested a review from sffc May 23, 2024 12:21
@Manishearth Manishearth removed their request for review May 23, 2024 16:14
@sffc
Copy link
Member

sffc commented May 24, 2024

It seems self-inconsistent that you're changing code to use turbofish in .parse::<Foo>() but not .collect::<Foo>()

components/calendar/src/provider.rs Outdated Show resolved Hide resolved
Comment on lines +717 to +721
Vec::from_iter(
WeekCalculator::try_new(&locale!("und").into())
.unwrap()
.weekend()
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a case where we need to increase indentation.

// Max indent 3
        WeekCalculator::try_new(&locale!("und").into())
            .unwrap()
            .weekend()
            .collect::<Vec<_>>(),

// Max indent 4
        Vec::from_iter(
            WeekCalculator::try_new(&locale!("und").into())
                .unwrap()
                .weekend()
        ),

@sffc
Copy link
Member

sffc commented May 24, 2024

I'm being somewhat picky with this review because the PR is literally entitled "Readability improvements" and I disagree that these are all consistently improvements.

@robertbastian
Copy link
Member Author

It seems self-inconsistent that you're changing code to use turbofish in .parse::() but not .collect::()

I would love to change code to Foo::from_str, but that's not in the prelude, whereas FromIterator is. I think it's more important to have expressions that don't rely on surrounding type context than to avoid turbofishes, because things like let x: Foo = "foo".parse().unwrap(); are a pain during refactorings.

@robertbastian robertbastian requested a review from sffc May 24, 2024 08:58
@sffc
Copy link
Member

sffc commented May 24, 2024

It seems self-inconsistent that you're changing code to use turbofish in .parse::() but not .collect::()

I would love to change code to Foo::from_str, but that's not in the prelude, whereas FromIterator is. I think it's more important to have expressions that don't rely on surrounding type context than to avoid turbofishes, because things like let x: Foo = "foo".parse().unwrap(); are a pain during refactorings.

So it sounds like you would be in support of let x = y.iter().collect::<Foo>(), right? Just not let x: Foo = y.iter().collect()? I'm onboard with that; type inference can cause problems. I just feel that .collect() is more conventional in Rust.

Evidence: 98.8k instances of from_iter( versus 1.3M instances of collect( on Rust GitHub.

@robertbastian
Copy link
Member Author

I'm not a fan of the turbofish if it can be avoided, I prefer using a constructor-like method (like Foo::from_string over .parse::<Foo>()). One issue with collections is that you have to add all generics, so you always end up with .collect::<HashMap<_, _>>(), which is just so many special characters compared to HashMap::from_iter(). I also like that from_iter is explicit about the allocation. In the course of this PR I found a few instances where there are unnecessary chained collects.

@sffc sffc added the discuss-priority Discuss at the next ICU4X meeting label May 29, 2024
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we're not in alignment on what is more readable, so I added "discuss-priority"

@robertbastian robertbastian removed the discuss-priority Discuss at the next ICU4X meeting label Jun 6, 2024
@robertbastian robertbastian deleted the turbo branch June 6, 2024 15:59
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.

None yet

2 participants