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

Consider "asList", "asMap", and "asSet" #77

Closed
matanlurey opened this issue Nov 8, 2016 · 7 comments
Closed

Consider "asList", "asMap", and "asSet" #77

matanlurey opened this issue Nov 8, 2016 · 7 comments

Comments

@matanlurey
Copy link
Member

I like this library, but one thing stopping me from using it is that I don't want to require other users to use built_collection, so I tend to only use it as an implementation detail and pass a toList/toMap/toSet as part of my API.

I would use asList/Map/Set, where they could be, for example:

List<T> asList() => new List<T>.unmodifiable(_backingList);
@davidmorgan
Copy link
Contributor

It's possible -- I'm not quite clear on how asList is better than toList, though.

The current toList is implemented using a copy-on-write wrapper, so it's cheap to create unless you actually then mutate it. So we have:

toList: fast, when you do a mutate, does a copy the first time
asList: fast, when you do a mutate, throws an exception

If the second is preferable in some cases, sure, we can add it.

FWIW, I try to pass around Iterable where possible. I think this is generally good practice. There are others who believe that Iterable should be used carefully because it might be a lazy iterable ... but for me, it's the people passing around lazy iterables who need to be careful, and the rest of us should use Iterable where possible as it's a nice interface.

Unfortunately there's no equivalent for map. (Iterable of values plus a lookup function is what I sometimes use in practice, but that's pretty horrible.)

@matanlurey
Copy link
Member Author

Really this is somewhere I wish we had a good zero-cost abstraction around a mutable list we don't want anyone to modify - perhaps something where asList could simply be implemented as:

asList() {
  var list = _backingList;
  assert(() {
    list = new List<T>.unmodifiable(list);
  });
  return list;
}

@davidmorgan
Copy link
Contributor

Well -- we can do better here.

new List.unmodifiable copies the list, but actually within BuiltList we know the list will not be modified.

So we could return a wrapper that throws on modify, like the current wrapper that copies on modify.

The question is whether the throw behaviour is really a useful thing to have :)

@matanlurey
Copy link
Member Author

That's fair.

I think I like having the throw behavior for developers to get failures when they use your API:

external Map getEmployeeMap();

void main() {
  // Throws StateError, due to the Map being returned being unmodifiable.
  getEmployeeMap().clear();
}

@davidmorgan
Copy link
Contributor

Hmm. For that example I think not throwing is better, then you can do e.g.:

final map = getEmployeeMap();
map['newEmployee'] = new Employee(); // CopyOnWriteMap does a copy here
store(map);

But maybe there are others where it's weird to let you mutate (well, appear to mutate) the collection? ... I guess that would be if you then assume the changes you make have an "action at a distance" effect on something:

final map = getEmployeeMap();
map['newEmployee'] = new Employee(); // CopyOnWriteMap does a copy here
final moreMap = getEmployeeMap();
// I think moreMap should contain 'newEmployee'

But for me that would be a surprising thing to do. Not sure. What do you think?

@matanlurey
Copy link
Member Author

I think copy on write at least gives the API producer safety, but the consumer will need to read the API documentation to know changes they are making effectively do nothing. That being said, I was more concerned about API producer side, so maybe this isn't a real priority for now.

@davidmorgan
Copy link
Contributor

I think it does make sense to have both, to give people a choice.

This should be trivial to implement, we can use UnmodifiableListView/UnmodifiableSetView/UnmodifiableMapView.

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

No branches or pull requests

2 participants