Updates to allow quick installation and compatibility with Python 2.xxx - Githubissues

bastibe / WebGL-Spectrogram

A smoothly animated spectrogram display in WebGL (FFT in Python/Tornado)

41 stars 10 forks source link

Updates to allow quick installation and compatibility with Python 2.xxx #1

Closed joshblum closed 9 years ago

joshblum commented 9 years ago

You can now install any dependancies with a single command to get the server up and running. Added an example.wav file to generate a spectrogram from. This file could be updated to match the screenshot.

bastibe commented 9 years ago

Very cool! Thank you for your contributions!

I'll gladly merge it, but please look at a few minor things first:

There are unnecessary entries in the .gitignore file. As far as I can see, we don't use C extensions, PyInstaller, pip, testing (although I should), translation, Django, Sphinx or PyBuilder.
The installation instructions use apt-get, which will work on Debian-based systems but not anywhere else. Similarly, I prefer to use conda instead of pip where possible, and Windows people might prefer to run the binary installers. The use of pip, brew, and apt-get should at least be mentioned in the README.
Please undo the indentation change of the source code. Fixing the flake8 suggestions is fine, but changing all indentation from four spaces to two spaces is not necessary and obscures your actual changes. I see that you actually fixed some bugs in there, for which I am very grateful. However, I almost missed that since every single line is marked as changed due to the indentation change.

joshblum commented 9 years ago

I removed a bunch of the unnecessary lines from the .gitignore but we are using pip (or at least that option is available) so I don't think it's worth it to go crazy removing every line that might not be used.
I've updated README.md and Makefile to talk a bit more about the installation and allow rpm based installations. I've never used conda for personal use and think it would be cool to have an install command that utilizes it, but that can be in a later diff.
Sorry about the indentation! I forgot I use 2-spaced. This file should be easy to see the diff of now :)
When merged, can you replace my example.wav file with the file you used for the screenshot?

bastibe commented 9 years ago

The file I used for the screen shot is https://dl.dropboxusercontent.com/u/98854/mann_short.wav. It's a recording of my own voice, so there shouldn't be any licensing issues.

bastibe commented 9 years ago

Thank you very much for your work on this! Easing the installation process of this really goes a long way to make this project useful!

I commented on one minor point. That point is my own fault though. Once that point is fixed, I'll merge.

joshblum commented 9 years ago

No problem! This has been really helpful for me and I'm glad to contribute :)