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

Named first runtime argument in resolve #19

Closed
wants to merge 4 commits into from

Conversation

ilyapuchka
Copy link
Collaborator

Resolves #18

@AliSoftware
Copy link
Owner

This means a 3.1.0 before the 4.0.0, right?

@ilyapuchka
Copy link
Collaborator Author

I don't mind bumping major version and release it together with other things that are in develop now. To release 3.1 first will be too much updates in short time. But do you think it will be ok just to remove old methods without deprecating them first? Currently in develop register(instance) is only deprecated. Then we can remove them all together in 4.1?

@ilyapuchka ilyapuchka force-pushed the feature/named-runtimer-arguments branch from 827bd4e to 1a3e2f6 Compare November 26, 2015 21:14
@ilyapuchka ilyapuchka added this to the 4.0.0 milestone Nov 26, 2015
@AliSoftware
Copy link
Owner

I think removing without deprecating might be maybe harsh but not that problematic either, as long as we provide a clear note in the CHANGELOG.

We can't deprecate in 4.0.x and remove in 4.1.x because that would break semantic versioning: when you remove a method and thus break the backward compatibility you have to bump the major version. So the solutions to chose from are:

  • deprecate it in 3.1 and remove in 4.0
  • deprecate in 4.0 and remove in 5.0
  • remove in 4.0 without deprecating it before

I think the first one would be the best, because typically when you do semver you use ~> 3.0 — so a pod update will update to 3.1 but not 4 until you change your Podfile manually. But if we dont plan to release a 3.1, then the last one won't be as problematic as if we were Apple or a big lib IMHO. After all we only have 50 stars so far, not 1k 😉

@ilyapuchka
Copy link
Collaborator Author

Ok, I see. But then if we deprecate it in 3.1 then we need to provide an alternative in the same version, don't we? For named arguments we have it, but for registering singleton we need to extract it from circular dependencies implementation. If it's ok I can do that.

@AliSoftware
Copy link
Owner

Totally.

I didn't realize that the alternative for register(instance:) was only provided by the Circular Dependencies PR, but to follow semver and be nice citizen with the API changes we should indeed extract the alternate register(.Singleton) from there so it can be included in 3.1 before we release Circular Dependencies in 4.0.

@ilyapuchka
Copy link
Collaborator Author

Ok, let's do that. I think it will be easier to start release/3.1.0 brunch from current master and do it there (we can't start it from develop cause circular dependencies are already merged in). And we need to close this then two.

@AliSoftware
Copy link
Owner

Yeah we messed up a bit with the release branches this time, it wasn't proper gitflow as we didn't have the roadmap yet, and should have created that 3.1.0 earlier or waited before merging the Circular Deps PR. Woops.

I think we should still create the branch from develop and not master, but from an earlier commit in develop — a commit before we merged circular dependencies PR. Like if we actually started that branch at that time in the git graph.
Then if we miss some commit from develop — other than the ones from the Circular Dependency but that were pushed after — we could still cherry-pick them into release/3.1.0 if needs be.
I think it would feel better than starting a release/3.1.0 from master, and more close to the gitflow philosophy.

@ilyapuchka
Copy link
Collaborator Author

Ok, will do that tomorrow.

@AliSoftware
Copy link
Owner

Well I just took a look at the git graph and realize that… I already did exactly that 4 days ago 😄

Branch origin/release/3.1.0 was created 4 days ago and points the the commit just after the release 3.0.0 and just before the Circular Dependencies branches started 😉

@ilyapuchka
Copy link
Collaborator Author

So can we close that?

@ilyapuchka
Copy link
Collaborator Author

Maybe it also worths adding inScope name to scope parameter in register? It can be also confusing if you use it together with tag:
register(tag: "tag", .Singleton) { ... }
vs
register(tag: "tag", inScope: .Singleton) { ... }
or
register(inScope: .Singleton) { ... }

@AliSoftware
Copy link
Owner

Mmh given that the Scope is a fixed enum, I'm not sure the confusion can be done like there was the confusion between tag and runtime argument before?

@ilyapuchka
Copy link
Collaborator Author

Yep, let's leave it like it is.
So I've made a pull request for 3.1.0 release - #21

@AliSoftware
Copy link
Owner

Cool! Will close this issue once I took time to merge #21 :)

@AliSoftware AliSoftware force-pushed the feature/named-runtimer-arguments branch from 1a3e2f6 to c620656 Compare November 29, 2015 23:23
@ilyapuchka
Copy link
Collaborator Author

We now don't need this as it is now in develop after merge of release/3.1.0 - 9778236

@ilyapuchka ilyapuchka deleted the feature/named-runtimer-arguments branch November 30, 2015 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants