Issue with formatting coordinates with colons #2183

[QuanTakeuchi]

Fixed the ugly appearance of colon when fields is specified to 2

[embray]

Just a really silly nitpick about putting spaces after commas, but other than that this looks good to me. I'll wait to let others more familiar with this code comment though.

[QuanTakeuchi]

@embray Hi, I am new to open source development and changing others code. This was the best I could think of to make it work without having to change a lot of code. But I hope this solves the issue.

[astrofrog]

@QuanTakeuchi - this looks good! As @embray pointed out, could you make sure you include spaces after the commas? That is, ('', '', '') instead of ('','','')? Just a matter of style conventions we follow (PEP8)

[QuanTakeuchi]

Done including the spaces.

[astrofrog]

@QuanTakeuchi - could you add the tests to test_angles.py instead of having a separate file? Also, you will need to remove the u unicode prefix from the test values otherwise it won't work on Python 3.1 and 3.2.

One last request - can you add an entry in the CHANGES.rst file, under the coordinates section in 0.3.2? You can look at the rest of the CHANGES.rst file to get an idea for what to write.

[QuanTakeuchi]

@astrofrog - made the changes you mentioned. Is the entry made by me in CHANGES.rst fine?

Also wouldn't it be better if I include the test in the file test_formatting.py, as it contains similar tests? I did include it in test_angles.py though.

[astrofrog]

@QuanTakeuchi - ah yes, I guess test_formatting.py would make more sense - good catch! The CHANGES.rst entry looks good.

@eteq - could you review this too?

[eteq]

looks fine to me, except that the most recent tests are still failing. I think this is a transient error, so I restarted the tests. Assuming they pass it looks good.

[mhvk]

Sorry for a late and somewhat nitpicky comment, but a nested if statement would seem more logical (and faster):

elif len(sep) == 1:
    if fields == 1:
        ...
    elif fields == 2:
        ...
    else:
        ...

and then also move the check that 1 <= fields <= 3 so that we know that fields is correct here.

[QuanTakeuchi]

@mhvk Did the changes you mentioned. I hope that's what you meant by moving the check.

[mhvk]

@QuanTakeuchi - yes, this was exactly what I meant. Since travis is happy, I'll merge...

[mhvk]

p.s. Thanks!