Swinject / Swinject-CodeGen

Generate code to make the use of Swinject type-safe and reduce duplicate identifiers for Services
MIT License
33 stars 5 forks source link

Review of Generated Code #3

Closed Lutzifer closed 8 years ago

yoichitgy commented 8 years ago

@Lutzifer plz let me know when the code is ready for review.

Lutzifer commented 8 years ago

This can be done, I do not have any open improvements/todos concerning the generated code atm

yoichitgy commented 8 years ago

I'll try reviewing in this weekend.

yoichitgy commented 8 years ago

Generated code has something like this:

func resolveInjectablePerson() -> InjectablePerson {
    return self.resolve(PersonType.self)! as! InjectablePerson
}

I think we don't need ! before as!. The generated code can be:

func resolveInjectablePerson() -> InjectablePerson {
    return self.resolve(PersonType.self) as! InjectablePerson
}
yoichitgy commented 8 years ago

Generated code has something like this:

func resolveInjectablePerson(foo: AType, bar: BType, baz: String, qux: String) -> InjectablePerson {
    return self.resolve(PersonType.self, arguments: (foo, bar, baz, qux))! as! InjectablePerson
}

I think it's more understandable if the first argument is labeled like this:

func resolveInjectablePerson(foo foo: AType, bar: BType, baz: String, qux: String) -> InjectablePerson {
    return self.resolve(PersonType.self, arguments: (foo, bar, baz, qux))! as! InjectablePerson
}

Then you can use the function like this:

let p = container.resolveInjectablePerson(foo: someFoo, bar: someBar, baz: "some baz", qux: "some qux")
yoichitgy commented 8 years ago

Generated code has something like this:

extension Resolvable {
    func registerInjectablePerson(registerClosure: (resolver: ResolverType) -> (InjectablePerson)) -> ServiceEntry<PersonType> {
        return (self as! Container).register(PersonType.self, factory: registerClosure)
    }
}

It uses (self as! Container), which expects the Resolvable instance must be Container type. I think it's more understandable to extend Container type for registration methods. (Resolution methods can stay in the extension of Resolvable.)

yoichitgy commented 8 years ago

I wrote three suggestions⬆️ What do you think about them?

Lutzifer commented 8 years ago

we had an issue with (self as! Container) in an earlier version of the code, but it seems to have vanished :-)

yoichitgy commented 8 years ago

@Lutzifer FYI: I'll rename Resolvable to ResolverType in Swinject 2.0.0.

https://github.com/Swinject/Swinject/pull/90 https://github.com/Swinject/Swinject/pull/87

CodeGen also needs migration to Swinject 2.0.0 later, but for now we can work on Swinject 1.x.

Lutzifer commented 8 years ago

we can bind the current version to swinject < 2.0.0, then update the generated code, release a new version and depend on swinject >= 2.0.0. I do not see any necessary migrations, as the code will simply be replaced.