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

rename listener to subscriber #134

Merged
merged 3 commits into from
Sep 19, 2014

Conversation

greg0ire
Copy link
Contributor

This PR renames listener classes to SomethingSubscriber. There are still some "listener" occurences in the README and in the DIC file, but I think changing them would be a BC-break.

@docteurklein
Copy link
Contributor

That would indeed be a BC break for those who manually defined services for them (or extended them, which shouldn't be done actually).

One solution would be to provide a temporary set of classes (that extends subscriber) and named like before. their ctor would raise a E_DEPRECATED warning.

@greg0ire
Copy link
Contributor Author

@docteurklein : I was more thinking about people using the DIC to inject services defined in your file, but after a closer look, I can see that you marked them as private, so there is no problem. Regarding the name of the classes, I don't think it is a BC-break : these classes are not part of your public API (which you could tag), they are used internally to do things. That's not like if we renamed the traits. I think you can merge this safely.

@docteurklein
Copy link
Contributor

right, lets merge it and note the renamings the UPGRADE.md file.

@docteurklein
Copy link
Contributor

could you add a UPGRADE.md file ?

@greg0ire
Copy link
Contributor Author

@docteurklein : with a 1.0.1 to 1.0.2 § ? I'll add it, but not now, because I think the private services could and should be renamed first, do you agree ? Plus, I'm a bit busy right now.

@docteurklein
Copy link
Contributor

right we can add it later

docteurklein added a commit that referenced this pull request Sep 19, 2014
@docteurklein docteurklein merged commit 1233a45 into KnpLabs:master Sep 19, 2014
@greg0ire greg0ire deleted the backwards-compatible branch September 19, 2014 09:58
@greg0ire
Copy link
Contributor Author

Ok, I'll try to do it tomorrow if I can.

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.

3 participants