Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dio/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ See the [Migration Guide][] for the complete breaking changes list.**
- Removes redundant warnings when composing request options on Web.
- Fixes boundary inconsistency in `FormData.clone()`.
- Support `FileAccessMode` in `Dio.download` and `Dio.downloadUri` to change download file opening mode
- Fix `ListParam` equality by using the `DeepCollectionEquality`.

## 5.7.0

Expand Down
9 changes: 7 additions & 2 deletions dio/lib/src/parameter.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'package:collection/collection.dart';

import 'options.dart';

/// Indicates a param being used as queries or form data,
Expand Down Expand Up @@ -26,9 +28,12 @@ class ListParam<T> {
identical(this, other) ||
other is ListParam &&
runtimeType == other.runtimeType &&
value == other.value &&
const DeepCollectionEquality().equals(value, other.value) &&
format == other.format;

@override
int get hashCode => value.hashCode ^ format.hashCode;
int get hashCode => Object.hash(
const DeepCollectionEquality().hash(value),
format,
);
}
1 change: 1 addition & 0 deletions dio/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ environment:

dependencies:
async: ^2.8.2
collection: ^1.16.0
http_parser: ^4.0.0
meta: ^1.5.0
path: ^1.8.0
Expand Down
32 changes: 32 additions & 0 deletions dio/test/parameter_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import 'package:dio/dio.dart';
import 'package:test/test.dart';

void main() {
test('ListParam equality for Map key', () {
Copy link
Member

Choose a reason for hiding this comment

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

Consider making a ListParam test group here.

final param1 = const ListParam(['item1', 'item2'], ListFormat.csv);
final param2 = const ListParam(['item1', 'item2'], ListFormat.csv);
final param3 = const ListParam(['item3', 'item4'], ListFormat.csv);
final param4 = const ListParam(['item2', 'item1'], ListFormat.csv);

final cache = {param1: 'Cached Response'};
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should directly compare their equality rather than using the map?

Copy link
Contributor Author

@suojae suojae Jan 27, 2025

Choose a reason for hiding this comment

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

Thanks for the suggestion!

I feel keeping the Map test might be useful if someone intends to use ListParam as a key in a Map. However, I’d like to make sure I’m on the right track. Is your suggestion aimed at simplifying the tests, or do you think the Map test is unnecessary in this case? If direct comparison is preferred, I’m happy to follow that direction ☺️

Copy link
Contributor Author

@suojae suojae Jan 27, 2025

Choose a reason for hiding this comment

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

I’ve updated the tests to use direct equality comparison instead of Map key checks. I think it aligns better with the readability and purpose of the tests.

Let me know if there’s anything else to refine!

commit log: 82e2f03

Copy link
Member

Choose a reason for hiding this comment

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

Is your suggestion aimed at simplifying the tests

Yes exactly. The PR is about the equality of the parameters so we should directly test their equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I see your point now, focusing directly on equality makes the purpose of the tests much clearer of the PR. I appreciate the feedback :)


expect(
cache.containsKey(param2),
true,
reason: 'param1 and param2 should be considered equal',
);

expect(
cache.containsKey(param3),
false,
reason: 'param1 and param3 should not be considered equal',
);

expect(
cache.containsKey(param4),
false,
reason:
'param1 and param4 should not be considered equal as order matters',
);
});
}
Loading