First round of changes #3

Closed
wants to merge 7 commits into
from

Projects

None yet

2 participants

@vicb
vicb commented Mar 5, 2014
  • fix several issues in Key
  • code cleanup

probably more changes to come in a further PR

@markovuksanovic markovuksanovic commented on the diff Mar 5, 2014
lib/injector.dart
@@ -56,7 +56,7 @@ class Injector {
Injector get root => _root;
- Set<Type> get types {
+ Set<Key> get types {
@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

Nice catch. Wonder how that didn't get caught by tests. Could you add a test which makes sure that correct type is returned?

@markovuksanovic markovuksanovic commented on the diff Mar 5, 2014
lib/injector.dart
@@ -79,7 +79,7 @@ class Injector {
}
dynamic _getInstanceByKey(Key key, Injector requester) {
- _checkTypeConditions(key.type);
+ _checkTypeConditions(key);
@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

In this case should probably be called _checkKeyConditions

@vicb
vicb Mar 5, 2014

Thought about that but really it checks a type condition

@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

I think _checkKeyCondition tells that the function checks conditions for key, while checkTypeCondition says more about how it checks - by checking the type. I think _checkTypeCondition better describes what the function does as opposed to how.

@vicb
vicb Mar 5, 2014

_checkTypeCondition better describes what the function does as opposed to how

so type is better ?

@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

I'd go for what (_checkKeyCondition).

@vicb
vicb Mar 5, 2014

I think it's better to describe what the function does, ie checks a Type condition rather than than how which is already described by the argument type (Key).

I think this is the usual way to name functions ?

@vicb
vicb Mar 5, 2014

Speaking about naming I think that this function is perfectly right but may be you want to rename _types and types to keys ?

@vicb
vicb Mar 5, 2014

Do you think it could be useful to have a getter for annotations while I'm at it ?

@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

I can't actually see a use case for it...

@vicb
vicb Mar 5, 2014

Thanks, just pushed the commit fixing type.

@markovuksanovic markovuksanovic and 1 other commented on an outdated diff Mar 5, 2014
int _hashCode;
- Key(this.type, {Iterable<Type> this.annotations: const []});
+ Key(this._type, {Iterable<Type> annotations})
+ : _annotations = annotations == null ? const [] : annotations;
+
+ Type get type => _type;
@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

I don't think we need this. One will not be able to modify it once it is set using the constructor. What was your motive for this change?

@vicb
vicb Mar 5, 2014

hashCode cache.

@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

But the type cannot be modified, I don't see how that affects hashCode. Am I missing something?

@vicb
vicb Mar 5, 2014

Sorry maybe I didn't get what you meant exactly ?

The hash code is computed once & cached. This cached value is no more valid once either the type or the annotations change. That's why I've made those RO - before it was possible to update the type & annotations leading to a bad hashCode (if it was cached before the change).

@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

Out of curiosity, how do you change a type in dart (in this context)? The variable is final, so once set it can't be changed. Yeah, you can change the contents, but how do you change contents of a Type?

@vicb
vicb Mar 5, 2014

You're right, type is final... I'll revert

@markovuksanovic markovuksanovic and 1 other commented on an outdated diff Mar 5, 2014
int _hashCode;
- Key(this.type, {Iterable<Type> this.annotations: const []});
+ Key(this._type, {Iterable<Type> annotations})
+ : _annotations = annotations == null ? const [] : annotations;
@markovuksanovic
markovuksanovic Mar 5, 2014 Owner

Not sure if this is really needed. Are there any performance impacts? Shouldn't be but just to be safe.

@vicb
vicb Mar 5, 2014

annotations was assumed not to be null in == and toString before.

  • it fixes 2 bugs,
  • by making the null check only once, perf could only be better
@vicb
vicb commented Mar 5, 2014

Just added a commit with the missing test for types type

@vicb
vicb commented Mar 5, 2014

marko, one question why 629 in _hashCode = 629 + type.hashCode; ? Thanks.

@vicb vicb refactor(Key): make type public to simplify the code
type is final anyway so it could not be changed
ba6e1cd
@markovuksanovic markovuksanovic commented on the diff Mar 7, 2014
test/main.dart
@@ -653,6 +653,13 @@ createInjectorSpec(String injectorName, InjectorFactory injectorFactory) {
void dynamicInjectorTest() {
describe('DynamicInjector', () {
+ it('should return a set of Keys when asked supported types', () {
@markovuksanovic
markovuksanovic Mar 7, 2014 Owner

Does this test fail if you return Set as is was before?. I tried something similar and the test did not fail. I'm suspecting something like type erasure in Java going on.

@vicb
vicb Mar 7, 2014

It would not fail, types are only informative in Dart.
It would fail if expect(injector.types, new isInstanceOf<Set<Type>>());.
I agree this is not the best possible test but I think the best we can do.

@markovuksanovic
markovuksanovic Mar 7, 2014 Owner

Not even in "checked" mode?

@vicb
vicb Mar 7, 2014

first thing checked: tests do run in checked mode

@markovuksanovic
Owner

@vicb Thanks for this. I'll just close this PR as this branch will not be merged. The single annotation version was chosen :) I've seen you opened another PR which Pavel is handling.

@vicb
vicb commented Mar 8, 2014

Yep thanks.

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