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 an empty unmodifiable map implementation #172

Closed

Conversation

donny-dont
Copy link

Adds an EmptyUnmodifiableMap implementation which provides a const constructor.

The Mixin classes within unmodifiable_wrappers.dart were changed from abstract class to mixin to modernize the code. This allowed the EmptyUnmodifiableMap to have a const constructor without also defining a const constructor in UnmodifiableMapMixin.

UnmodifiableMapMixin was missing a handful of methods that change a Map. The mixin was synced to _UnmodifiableMapMixin from dart:collection.

Removed the first and last setter since they don't correspond to Map methods.
@google-cla google-cla bot added the cla: yes label Dec 11, 2020
@donny-dont
Copy link
Author

@kevmoo @natebosch I think this would help simplify some of the shelf stuff around context and headers.

class Context extends UnmodifiableMapView<String, Object> {
  factory Context(Map<String, Object> from) {
    if (from is Context) return from;
    if (from.isEmpty) return Context.empty();
    return Context._(from);
  }
  const factory Context.empty() = _EmptyContext;
  Context._(Map<String, Object> from) : super(from);
}

class _EmptyContext extends EmptyUnmodifiableMap<String, Object>
    implements Context {
  const _EmptyContext();
}

@kevmoo
Copy link
Member

kevmoo commented Dec 11, 2020

Ooo!

Tests?
Changelog entry?
Version bump?

@donny-dont
Copy link
Author

Sure! Wanted to make sure that the changes were acceptable before writing tests.

Version would be 1.15.0-nullsafety.6? I'm assuming after null safety lands the package is being bumped to 2.0 right?

@kevmoo
Copy link
Member

kevmoo commented Dec 11, 2020

Let @natebosch chime in before you do any more...

@natebosch
Copy link
Member

natebosch commented Dec 11, 2020

Could we skip the extra class and use const {}?

class Context extends UnmodifiableMapView<String, Object> {
  factory Context(Map<String, Object> from) {
    if (from is Context) return from;
    if (from.isEmpty) return empty;
    return Context._(from);
  }
  static final empty = Context._(const {});
  Context._(Map<String, Object> from) : super(from);
}

Or, if we make the UnmodifiableMap constructor const it opens up options to get you closer to the pattern you had if you strongly prefer that:

class _EmptyContext extends UnmodifiableMapView<String, Object>
    implements Context {
  const _EmptyContext(): super(const {});
}

or without the empty class, but keeping the Context.empty() as a constructor and not singleton (let's imagine you need to handle generics even though this example doesn't)

class Context extends UnmodifiableMapView<String, Object> {
  factory Context(Map<String, Object> from) {
    if (from is Context) return from;
    if (from.isEmpty) return Context.empty();
    return Context._(from);
  }
  const Context.empty() : this._(const {});
  const Context._(Map<String, Object> from) : super(from);
}

I don't think I see a reason we'd need this class in this library. I'd be happy to explore whether we can make the existing constructor const.

@natebosch
Copy link
Member

I'm assuming after null safety lands the package is being bumped to 2.0 right?

No, this and a few other very core packages are migrating to null safety without a major version bump because an early choice by the Flutter framework makes it infeasible to do breaking releases.

@mit-mit
Copy link
Member

mit-mit commented Sep 13, 2021

Closing as assumed stale

@mit-mit mit-mit closed this Sep 13, 2021
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

4 participants