dflydev / dflydev-embedded-composer

Embed Composer into another application
MIT License
71 stars 17 forks source link

remove unused use statements and a bit of cs #6

Closed cordoval closed 10 years ago

cordoval commented 10 years ago
Q A
Bug Fix? n
New Feature? n
BC Breaks? n
Deprecations? n
Tests Pass? n
Fixed Tickets
License MIT
Doc PR

Sent using Gush

cordoval commented 10 years ago

this may look like sweeping to you @simensen but basically is:

simensen commented 10 years ago

@cordoval You know me pretty well. :)

I wish you would try to put these things into separate PR. It seems like you understand that I think this is too much at one go, so I think you know the reasons why.

1) If I wanted to move from PSR-0 to PSR-4 (I'm not even sure I do...) I would not move the files. I would instead add the additional existing directories after src/ so that the directory structure could stay the same. I am not a fan of trimming directories. Further, I'd argue that this is not BC to change the directory structure.

2) Adding 2014 to the licenses as nice, thank you. However, this should be its won PR.

3 & 4) I'm fine with this but it would be nice if this was its own PR.

I'm also OK with removing unused use statements. If that was its own PR, I'd probably merge it right away. Same with 2), 3), and 4)... if they were each their own PR, I'd probably merge them right away.

Is there a reason why you prefer to do these things in one big commit/PR? Are you running automated tools for some of this? Can they be tweaked to do things in different sections?

cordoval commented 10 years ago

i will launch 3+ PRs, thanks for your time of responding. I was wrong and thought in advance you were not going to merge these, but seeing your willingness I am on the works now.

simensen commented 10 years ago

Sure thing. It is mostly that I need to make sure it is easy for me to see exactly what is going on. I don't have as much time as I'd like to go through these things. Makes me sad. But it is reality. I'll look forward to seeing the other PRs. :)

cordoval commented 10 years ago
1) If I wanted to move from PSR-0 to PSR-4 (I'm not even sure I do...) I would not move the files.
 I would instead add the additional existing directories after src/ so that the directory structure could
 stay the same. I am not a fan of trimming directories. Further, I'd argue that this is not BC to change
 the directory structure.

I only contest this point. Having less folders help to navigate on phpstorm faster or easily for development. Also for writing tests and such. I think there is no need to click and expand directories unless they are many and they fork at the early levels of the folder structure. But here it is almost serially. I know that github allows this shortcut but i am talking as a developer on a unix folder system and editor.

simensen commented 10 years ago

@cordoval I know the arguments. I would say there is a high probability that I'll change my tune sometime in the next few years. However, at this point in time, the upside of not having to click through an extra couple of directories in an IDE does not make up for the fact that I can mentally know exactly in which directory and classes will live in. I'll think on merging #13 if and when I'm ready to make that change.

Thanks for your contributions!

cordoval commented 10 years ago

I totally agree then, and will leave it sitting there for further pondering. I am preparing some other PRs.