search

drsounds / mingus

Automatically exported from code.google.com/p/mingus
GNU General Public License v3.0
0 stars 0 forks source link

Inconsistent Indentation in Source Files #91

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Hello,
    I have noticed an inconsistent indentation in the source files, where
most of the code has tab-indentation, but some areas have space-based
indentation. This is liable to cause problems in the future.

Suggestions:
    * including a line for vim and emacs users to specify the usage of tabs
(for vim this would be a line "# vim set noexpandtab" at the top of the file. 
    * Would it be possible to create a hook script for SVN that would check
that the committed files all begin with tab-indentation?

Cheers,
Arun.

Original issue reported on code.google.com by arunchag...@gmail.com on 25 Nov 2009 at 6:08

GoogleCodeExporter commented 9 years ago 
I also wanted to output some stats on the number of such violations (Though 
many of
these will be due to the license header at the top, which is 10 lines:

[teju@vimzard mingus]$ grep "^ " -c -R --include="*.py" .
./doc/generate_wiki_docs.py:10
./unittest/test_progressions.py:0
./unittest/test_fluidsynth.py:2
./unittest/run_tests.py:12
./unittest/test_Composition.py:0
./unittest/test_diatonic.py:1
./unittest/test_Bar.py:0
./unittest/test_notes.py:0
./unittest/run_lilypond_tests.py:10
./unittest/test_value.py:0
./unittest/test_NoteContainers.py:42
./unittest/test_chords.py:7
./unittest/run_fluidsynth_tests.py:10
./unittest/test_Note.py:19
./unittest/test_meter.py:0
./unittest/test_LilyPond.py:0
./unittest/test_tunings.py:37
./unittest/test_Track.py:0
./unittest/test_fft.py:7
./unittest/test_intervals.py:32
./unittest/test_Instrument.py:0
./unittest/test_scales.py:0
./unittest/test_tablature.py:2
./unittest/test_Suite.py:0
./mingus_examples/pygame-piano/pygame-piano.py:0
./mingus_examples/improviser/improviser.py:0
./mingus_examples/pygame-drum/pygame-drum.py:0
./mingus_examples/play_progression/play-progression.py:0
./googlecode_upload.py:172
./setup.py:31
./mingus/midi/fluidsynth.py:96
./mingus/midi/MidiTrack.py:10
./mingus/midi/MidiEvents.py:0
./mingus/midi/__init__.py:10
./mingus/midi/Sequencer.py:112
./mingus/midi/MidiFileOut.py:12
./mingus/midi/pyFluidSynth.py:181
./mingus/midi/SequencerObserver.py:77
./mingus/midi/MidiFileIn.py:12
./mingus/extra/MusicXML.py:171
./mingus/extra/LilyPond.py:18
./mingus/extra/__init__.py:10
./mingus/extra/tunings.py:364
./mingus/extra/fft.py:113
./mingus/extra/tablature.py:331
./mingus/__init__.py:10
./mingus/core/progressions.py:10
./mingus/core/chords.py:11
./mingus/core/__init__.py:10
./mingus/core/scales.py:10
./mingus/core/mt_exceptions.py:12
./mingus/core/value.py:10
./mingus/core/intervals.py:20
./mingus/core/notes.py:10
./mingus/core/diatonic.py:12
./mingus/core/meter.py:10
./mingus/containers/Bar.py:16
./mingus/containers/NoteContainer.py:69
./mingus/containers/Instrument.py:11
./mingus/containers/__init__.py:10
./mingus/containers/mt_exceptions.py:10
./mingus/containers/Track.py:10
./mingus/containers/Suite.py:15
./mingus/containers/Note.py:63
./mingus/containers/Composition.py:10

Original comment by arunchag...@gmail.com on 25 Nov 2009 at 6:16

GoogleCodeExporter commented 9 years ago 
Yes, the indentation is messy and should be changed or at least fixed. I'd 
personally
like to switch to 4 spaces if we are going through all the files, to avoid 
confusion
and to comply to PEP-8. This would probably involve some more manual work 
though so
let me know what you think.

Original comment by Rhijnauwen@gmail.com on 25 Nov 2009 at 6:26

GoogleCodeExporter commented 9 years ago 
Hey,
    I've looked around for a script that would do the job, and found PythonTidy. I've
attached the script + a bash script that should take care of rectifying all the 
code.
I've tested it with the unittests, and they pass fine. 

    Also, I'd like to ask how I should go about committing my changes, and attaching
it to the issue tracker.

Cheers,
Arun.

Original comment by arunchag...@gmail.com on 26 Nov 2009 at 12:22

Attachments:

GoogleCodeExporter commented 9 years ago 
Please use this tidy-all.sh instead - it preserves the file permissions.

Original comment by arunchag...@gmail.com on 26 Nov 2009 at 12:25

Attachments:

GoogleCodeExporter commented 9 years ago 
Nice work! The script does seem to fix all indentation errors, but 
unfortunately it
also messes with the docstrings, which are used by the (reference) documentation
generator. Specifically, it removes the trailing backslashes and inserts 
indentation
and this breaks the formatting of the code examples and explanations embedded 
in the
strings. The doc generator would be hard to change, because it wouldn't know 
when to
insert and remove line breaks and spaces; so unfortunately this means we need to
leave the docstrings alone.

It seems that the offending code in PythonTidy is the put_doc method beginning 
on
line 1706. This should just output the docstring as is. Setting 
LEFTJUST_DOC_STRINGS
and WRAP_DOC_STRINGS to False fixes the line break issue, but still removes the
pending slash character (I think it would have to parse the file itself to be 
able to
retain it, because it's manipulating the AST now). So this is still not really 
an option.

What we need in put_doc is something that does this: get the docstring as 
unicode,
split lines, wrap the lines that are too long (>80 characters) by inserting a
backslash and a linebreak after a space and leave the rest alone (no indenting, 
etc).

> Also, I'd like to ask how I should go about committing my changes, and 
attaching
it to the issue tracker.

For example, if this is fixed you could just 
   svn ci -m "Replaced tabs with spaces in every module. Closes issue 91." 

This will automatically close the issue and send some mails to the dev list:
http://groups.google.com/group/mingus-devel 
See also
http://code.google.com/p/support/wiki/IssueTracker#Integration_with_version_cont
rol
for more info on tracker integration.

Hope this helps, 
Bart

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 7:58

GoogleCodeExporter commented 9 years ago 
I've changed the way WRAP_DOC_STRINGS works and this seems to do the right 
thing. See
the patch.

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 8:19

Attachments:

GoogleCodeExporter commented 9 years ago 
Disregard that...still not wrapping correctly.

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 8:23

GoogleCodeExporter commented 9 years ago 
This patch does the right thing for function docstrings, but the consequence is 
that
it now also messes with the header, because it can't differentiate between the 
two. 

A (slightly hacky) solution to this would be to check for the header in put_doc 
(a
newline and 80 ='s) and leave that alone in its entirety. I might code this up 
later,
because I don't know what else we could do — except for using ggVG= in vim 
for every
file, but this is kind of tedious and not as maintainable as a script that we 
can use
as hook.

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 8:45

Attachments:

GoogleCodeExporter commented 9 years ago 
Here is my last patch against the PythonTidy-1.19 file you attached. This takes 
care
of the header strings as well. Hopefully this will cover all the cases, but I'm 
not
100% certain of that (I'm not sure if docstrings have been used in other 
manners).
Let me know what you think.

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 12:04

Attachments:

GoogleCodeExporter commented 9 years ago 
Hey,
    Ah, sorry, I didn't check for the example code formatting. The script's
modifications work great. I just changed one line to make it a little less 
hacky - I
dropped the "="*80 and made the COL_LIMIT=80. The output should be the same, 
and does
seem to be that way as well.

Cheers,
Arun.

Original comment by arunchag...@gmail.com on 26 Nov 2009 at 1:23

GoogleCodeExporter commented 9 years ago 
Hey,
    Ah, sorry, I didn't check for the example code formatting. The script's
modifications work great. I just changed one line to make it a little less 
hacky - I
dropped the "="*80 and made the COL_LIMIT=80. The output should be the same, 
and does
seem to be that way as well.

Cheers,
Arun.

Original comment by arunchag...@gmail.com on 26 Nov 2009 at 1:26

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks that works nicely. This means the whole if statement can now also be 
removed.
See my patch.

That should do it, I think. You can now use your tidy-all script and commit the
changes if you want.

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 2:06

Attachments:

GoogleCodeExporter commented 9 years ago 
Closed by http://code.google.com/p/mingus/source/detail?r=579. 

(You added issue 90 instead of 91 to the commit message.)

Original comment by Rhijnauwen@gmail.com on 26 Nov 2009 at 2:35