Skip to content

Conversation

@arthurbcd
Copy link

sortedBy is one of the best extensions of this package.

It allows us to easily sort, but we can't set if the order comparaison is ascending/descending, it always defaults to "natural ordering".

This PR simplifies this process so we can simply decide it based on the added ascending parameter in the sortedBy extension.

  List<T> sortedBy<K extends Comparable<K>>(
    K Function(T element) keyOf, {
    bool ascending = true,
  })

This is useful, because without it, we would have to do additional operations bringing either more boilerplate or additional computations. By adding this named boolean we can make it straightforward.

For many developers, especially those who might not be as familiar with comparators and more advanced functional programming concepts, having a straightforward boolean parameter makes the API more accessible and user-friendly.

The boolean ascending is a natural state. It’s a common need to toggle sort order, and doing so with a single extension method and a simple parameter makes the code more readable and maintainable. Adopting this familiar pattern in sortedBy makes it feel more intuitive and consistent, making the intent of the code clearer.

Flutter itself uses this pattern for sorting, as we can see in flutter/lib/src/material/data_table.dart:

/// Signature for [DataColumn.onSort] callback.
typedef DataColumnSortCallback = void Function(int columnIndex, bool ascending);

I find this extension incredibly useful and have always relied on a version of it. I dedicated time to updating it for the community because I believe it addresses a fundamental and widely shared need across projects.

Thank you for taking the time to review this PR—I hope it proves valuable!

Added tests ✅
Retrocompatible ✅
Not a breaking change ✅


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@google-cla
Copy link

google-cla bot commented Dec 6, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mosuem mosuem requested a review from a team April 17, 2025 13:01
Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

My initial inclination is to not add this - I don't think we have a similar argument in other sorting methods, we instead expect callers to arrange an appropriate compare callback.

Here I think it's better to ask users to call sortedByCompare for this use case.

cc @lrhn WDYT?

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

I think it's an OK feature.
I would reverse the parameter name (to make he default value false) and give every sorting function the same treatment (if possible).

/// when [ascending] is `false`.
List<T> sortedBy<K extends Comparable<K>>(
K Function(T element) keyOf, {
bool ascending = true,
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer to name it descending, so the default is false.
Just because it's usually best to have "nothing" mean false.
Then it feels like you are opting in to the non-default behavior. Using ascending: false feels ... conflicted. You are opting out of the default behavior, but that doesn't directly say what it opts in to.

Copy link
Author

Choose a reason for hiding this comment

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

I understand the concern about double negatives with ascending: false. However, I'd lean towards ascending: true in this case because:

  1. Flutter's DataTable already uses sortAscending: true as the default, and DataColumnSortCallback uses bool ascending in its signature
  2. In sorting contexts across Flutter/Dart, I feel developers would expect the ascending parameter

That said, I recognize the clarity benefit of descending: true for opt-in behavior. If you still prefer descending: false after considering these points, I'm happy to update it accordingly. :)

@mosuem
Copy link
Member

mosuem commented Oct 7, 2025

@arthurbcd do you still intend to land this PR? If so, this is a friendly ping!

@brianquinlan
Copy link
Contributor

@arthurbcd do you still intend to land this PR? If so, this is a friendly ping!

I think that we are waiting for you to address the review feedback.

@brianquinlan brianquinlan added the needs-info Additional information needed from the issue author label Nov 3, 2025
@github-actions github-actions bot removed the needs-info Additional information needed from the issue author label Nov 10, 2025
@github-actions
Copy link

PR Health

License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

This check can be disabled by tagging the PR with skip-license-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Breaking changes ⚠️
Package Change Current Version New Version Needed Version Looking good?
collection Breaking 1.19.1 1.20.0-wip 2.0.0
Got "1.20.0-wip" expected >= "2.0.0" (breaking changes)
⚠️

This check can be disabled by tagging the PR with skip-breaking-check.

Coverage ✔️
File Coverage
pkgs/collection/lib/src/iterable_extensions.dart 💚 100 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

Changelog Entry
Package Changed Files
package:collection pkgs/collection/lib/src/iterable_extensions.dart

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants