codeeu / coding-events

A Django app for adding EU Code Week events and showing them on a map.
http://events.codeweek.eu
MIT License
17 stars 36 forks source link

Fix indentation and add status=="Approved" to event filter #422

Closed michelesr closed 8 years ago

michelesr commented 8 years ago

Also disabled a unit test that check scoreboard content, given that now scoreboard is filtered to countries with at least 1 event and without events inserted in the testdb is blank

michelesr commented 8 years ago

Commands used to fix indentation:

# replace tabs with 4 spaces
find . -name '*.py' -exec sed -i 's/\t/    /g' {} \;
# remove trailing backspaces
find . -name '*.py' -exec sed -i 's/ *$//g' {} \;
michelesr commented 8 years ago

ping @mitio

michelesr commented 8 years ago

Fix #421

mitio commented 8 years ago

@michelesr I think this sed pattern can potentially do harm to some files as it replaces tabs in any place in the line, not only at the start. Maybe we should instead anchor it to the beginning of the line?

Also, how about we add \t to the one for removing trailing spaces as well? Might have some trailing tabs here and there.

Another point--why didn't you separate the whitespace changes from the other ones? I think it's a better idea (by using feature branches).

Are you going to change the patterns or should I do the replacement?

michelesr commented 8 years ago

@michelesr I think this sed pattern can potentially do harm to some files as it replaces tabs in any place in the line, not only at the start. Maybe we should instead anchor it to the beginning of the line?

Appending the regex for the beginning of the line will not remove multiple tab i guess, and even if you use the + wildcard you can't know how many spaces to apply ...

Also, how about we add \t to the one for removing trailing spaces as well? Might have some trailing tabs here and there.

If the command to replace tabs with spaces is performed well, then tabs are replaced with spaces and trailing spaces are removed with the next command

Another point--why didn't you separate the whitespace changes from the other ones? I think it's a better idea (by using feature branches).

That's a good point, i was thinking to split in two commits (one for the indentation, and one for the filter hotfix)

michelesr commented 8 years ago

@mitio: if you got ideas for sed pattern post here and i'll give a check

mitio commented 8 years ago

@michelesr I think this sed pattern can potentially do harm to some files as it replaces tabs in any place in the line, not only at the start. Maybe we should instead anchor it to the beginning of the line?

Appending the regex for the beginning of the line will not remove multiple tab i guess, and even if you use the + wildcard you can't know how many spaces to apply ...

Agreed about the count of replacements.

How about we use autopep8 or this script instead?

Also, how about we add \t to the one for removing trailing spaces as well? Might have some trailing tabs here and there.

If the command to replace tabs with spaces is performed well, then tabs are replaced with spaces and trailing spaces are removed with the next command

If we're removing only indentation spaces, tabs can still be left at the end of the line.

michelesr commented 8 years ago

Ok... then let's try with the autopep8 and remotion of trailing spaces and tabs.

Working on it.

michelesr commented 8 years ago

Closing this PR... i'll make another with cleaner commits

mitio commented 8 years ago

:+1: