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

CanonicalizedMap: new copy, toMap and toMapOfCanonicalKeys methods #261

Merged
merged 6 commits into from May 18, 2023

Conversation

gmpassos
Copy link
Contributor

  • copy: copies a CanonicalizedMap instance without recalculating the canonical values of the keys.
  • toMap: creates a Map<K,V> (with the original key values).
  • toMapOfCanonicalKeys: creates a Map<C,V> (with the canocalized keys).

…thods.

- `copy`: copies a `CanonicalizedMap` instance without recalculating the canonical values of the keys.
- `toMap`: creates a `Map<K,V>` (with the original key values).
- `toMapOfCanonicalKeys`: creates a `Map<C,V>` (with the canocalized keys).
@natebosch
Copy link
Member

These APIs look relatively straightforward and I don't have concerns about adding them.

For clarity, @gmpassos - could you describe the use case you have in mind for these?

cc @lrhn

@lrhn
Copy link
Member

lrhn commented May 17, 2023

I'm always a little worried when adding methods to an implementation of a standard interface like Map.
In this case, the methods are related to the implementation strategy, so it makes sense.

It's also reasonable functions.

So thanks for the contribution! Can you update the changelog and the pubspec version as per https://github.com/dart-lang/.github/blob/main/PULL_REQUEST_TEMPLATE.md ?

It should just be a minor version increment, and a changelog entry for that version saying which methods are added.

@gmpassos
Copy link
Contributor Author

These APIs look relatively straightforward and I don't have concerns about adding them.

For clarity, @gmpassos - could you describe the use case you have in mind for these?

cc @lrhn

Currently the most common use case is:
https://github.com/dart-lang/http_parser/blob/master/lib/src/case_insensitive_map.dart

It's all about performance. If you use any Map based on CanonicalizedMap and require a copy of it (which is a common operation), you may encounter a bottleneck due to the recalculation of the canonical keys.

It is also important to have methods for transforming it into a regular Map (toMap and toMapOfCanonicalKeys), in case you want to eliminate the overhead associated with the canonical function.

Best regards

- `CanonicalizedMap`:
  - Added methods:
    - `copy`: copies an instance without recalculating the canonical values of the keys.
    - `toMap`: creates a `Map<K,V>` (with the original key values).
    - `toMapOfCanonicalKeys`: creates a `Map<C,V>` (with the canonicalized keys).
- lints: ^2.0.1
@gmpassos
Copy link
Contributor Author

It should just be a minor version increment, and a changelog entry for that version saying which methods are added.

Done 👍

CHANGELOG.md Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
gmpassos and others added 2 commits May 18, 2023 16:16
Co-authored-by: Nate Bosch <nbosch1@gmail.com>
Co-authored-by: Nate Bosch <nbosch1@gmail.com>
@natebosch natebosch merged commit f949b09 into dart-lang:master May 18, 2023
5 checks passed
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

3 participants