search

Mach5 / supersonic

Open-source web-based media streamer and jukebox fork of Subsonic. Supports MP3, OGG, AAC and other streamable audio and video formats. Runs on Windows, GNU/Linux and Mac using Java.
226 stars 61 forks source link

optimize js and css files #35

Closed nicolasberens closed 12 years ago

nicolasberens commented 12 years ago

added yuicompressor-maven-plugin to the build process (the many changes in the file are because of the refoctoring with eclipse) this is the pull request to my issue #33

timoreimann commented 12 years ago

I find it very difficult to tell apart what is new code/configuration and what is just re-indentation. As far as I can tell, your changes are rather small but it's just hard to spot.

Would you mind re-applying your changes onto the non-changed (master) copy of the POM file and matching indentation manually? I think this would make it much easier to see the diff not just for the sake of acknowledging this push request but to allow to understand the code changes in the future too. If you really wanted to "fix" the indentation this should come as another commit/push that's not interweaved with feature enhancements. (My personal view though is to stick to non-Eclipse-conform indentation to support file history traceability but that's just my opinion.)

porkcharsui commented 12 years ago

I have to agree with @timoreimann on his comment about the large delta in the POM.xml for such a minor change. We need to avoid reformatting indentation on files that are originally from Subsonic. Accepting the commit as is will cause issues merging future upstream changes.

nicolasberens commented 12 years ago

here you are

timoreimann commented 12 years ago

In your most recent commit 991d6ca, there are still 126 additions and 128 deletions with a lot of identical code except for indentation. To my understanding, however, the changes should comprise a rough 10 to 20 lines.

Could you please re-try once more in order to strip the diff down to the absolute minimum? Otherwise, we'll get into big trouble regarding upstream merging just as porkcharsui explained.

nicolasberens commented 12 years ago

well the latest commit reverses the changes of the commit before..

nicolasberens commented 12 years ago

now i have done something strange, i will close this pull request

timoreimann commented 12 years ago

Awww sorry, missed to see the nature of your revert. My bad, I shouldn't be revising pull requests when tired.

Let us know when you fixed your error, I'll merge in your request then.

timoreimann commented 12 years ago

Issue #33's thread will conclude the topic.