-
Notifications
You must be signed in to change notification settings - Fork 177
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
[Suggestion] Remove the Interface suffix in interface names #12
Comments
👍 makes a lot of sense. |
i2399: The happy path tests for sign up scenario
tune composer.json
👍, let's try to do this sometime soon. Again a PR would be great. Could be combined with #2 maybe. |
👍 |
It is possible I could do this if it isn't already being taken care of by someone else. |
hey Beau @simensen have you started this? |
@kelvinj I have not. :) I wanted to make sure someone else wasn't already working on it. :) Will probably be a considerable amount of work (or at least a considerable amount of renames and whatnot) so I didn't want to double-up on that effort. |
@simensen Hmmm… might put phpstorm to work and see what the upshot is… |
@kelvinj Have at it. :) |
One thing to keep in mind in terms of BC is that these changes will require renaming some existing (non-Interface) classes. For instance, there is currently a While it isn't a huge deal to rename the conflicted classes (especially using PHPStorm), it does create a problem if any projects are using the classes directly. The abstract |
You could easily say the same about interfaces themselves. The approach I'm taking is to rename the interface to represent what it does, not what it is. For example, This does mean that the names of the default implementations can remain as they are. Separately, I was intending to raise the issue of some abstract classes being preceded with 'Abstract' while others are not. Not a huge deal but would be good to have a general rule of thumb. I'll try to submit a PR either later or tomorrow am, and then people can tear it apart as they wish. |
Yep, either way works and again this isn't a huge deal. Proper naming will alleviate some of the issues. Just wanted to mention it for posterity :) |
Well, these default implementations should also be named according to what makes them special. There is no reason to have a priviledged name for them. |
@stof I agree entirely with your stance when applied to concepts that naturally have multiple implementations. |
Having started #23 I think the one question that remains open to me is… is this kind of change important enough right now? Qandidate are likely the only ones with production apps that rely on it, and it's their baby, but then it's also relevant to anyone with an interest in contributing. For me, even though I find interfaces with this naming easier to understand as I navigate the code, it isn't a show stopper. I'm more interested right now, for example, working on different types of projection backends. |
It might be better if issue #2 is resolved before removing the suffix. That issue might cause other naming collisions. After that we should look again at the namespace suffix, renaming interfaces to have more sensible names might even be step 3? |
@wjzijderveld so for example, changing What are you advocating to be step 2? |
Namespace suffix sounds like |
Yes, that would be step 1. With step 2 I meant this issue (#12), so only drop the Interface suffix and rename the Interface if there are collisions. Splitting it would also split the discussions about naming, making it a bit more manageable. |
ok cool, I'm all for making stuff more manageable. |
such change can only be done now: it is a BC rbeak, so doing it later in time will become impossible (until a next BC-breaking major version). |
👍 Indeed, I'm currently in the process of trying to get at least one project up and running on Broadway. It will be a pain to make these types of changes a month or two from now. That said, BC breaks are going to happen eventually and we can leverage semver for that to a certain extent. We can target renaming the interfaces for the next major release (or minor release since we're on 0.x or whatever). Still, doing it soonish would probably still be my vote. I'm happy to help however I can. |
@simensen such renaming are hard BC breaks (lots of changes to do everywhere), so it is much easier to do it soonish. This is why I opened the suggestion as soon as it has been open-sourced |
@stof @asm89 @simensen @henrikbjorn @wjzijderveld @mbadolato Opinions please, on how would you like to see the naming changed around an
On reflection, for me, it's either 1 or 4… probably erring towards 4. I think it would be well worth writing something similar to the PSR coding conventions to clarify for other contributors. |
I would have 2 but if that isnt possible then 4. I really like interfaces that are natural to read. |
My order of preference would be: 4, 2, 1. As I understand it, 1 is pretty close to 0 / current, the only difference being that any abstract classes that are not already named Abstract* would be renamed? I think that would be helpful and more consistent so even that would be a bump in the right direction. |
I would vote for 4. The option 3 does not only half of the work IMO. Note that I would not enforce interface names to be nouns through, as it might depend on the use case. |
I vote for 0 or 4. I have no problem with the Interface suffix, but |
I would go with 4 or current But 👍 on documenting it, there probably will be discussions about this in the future... But nobody said naming things is simple :) |
+1 on 4, though I'm not sure about the name What do you guys thing? (cc @qandidate-labs/broadway) |
Hey everyone, Has this been burried? Can we brint it back to life? as there seemed to be a consensus on approch number 4... If so, tell me, I'd be happy to help and submit a PR on that matter. |
Not buried, but a bit low on prio. To be honest, I'm not sure what the best approach is here. |
It would be great to drop the suffix. |
Sorry for bumping it again, but wouldn't it be too late to rename interfaces later? This is a hard BC break. |
we are about to tag 1.0, so I guess it's now or never time for this change. Does anyone want to create a PR? It would be highly appreciated. If not we should bury this as a known issue. 😑 |
@othillo I can try it. However, it makes sense to make decision regarding the following classes as they conflict with corresponding interfaces:
Do you really need |
after discussion with @wjzijderveld we decided on new interface names, see #308 |
See http://verraes.net/2013/09/sensible-interfaces/ for the argumentation. Note that most components are already using a descriptive name for the provided implementation rather than making it the priviledged one (for instance
class SimpleEventBus implements EventBusInterface
)Doing such should be done quickly, to avoid fighting with BC concerns with the current interface names.
The text was updated successfully, but these errors were encountered: