MPAS-Dev / MPAS

Repository for private MPAS development prior to the MPAS v6.0 release.
Other
4 stars 0 forks source link

Correct COMPASS python script formatting #1472

Closed pwolfram closed 6 years ago

pwolfram commented 6 years ago

Current, python scripts in the ocean COMPASS test suite framework use tabs and do not use standard python conventions.

This can readily be mediated via use of mode lines, e.g.,

# vim: ai ts=4 sts=4 et sw=4 ft=python

The current formatting makes it hard to edit the scripts using standard text editor approaches, e.g., vim or spyder.

xylar commented 6 years ago

I could take care of making all the COMPASS python PEP8 compliant. I can put in the mode lines, too, though I must admit that I find writing formatting like folds and mode lines annoying since I don't use them.

pwolfram commented 6 years ago

@xylar, thanks for following up here. I'm happy to make these changes and plan to do them post completing #1470. However, could I ask for you to do a review once I've done so?

I'll make sure it passes testing via the linter, spyder, and modelines to be as general as possible but verification that the changes are reasonable on your end will be greatly appreciated.

xylar commented 6 years ago

Okay, sounds fine. I've already started on this but it sounds like you have, too.

Most challenging fixes that I came across: excepts without error names. Most are KeyErrors but some are subprocess.something and others are ConfigParser.something. I'll try to help figure those out once you create a PR.

pwolfram commented 6 years ago

@xylar, I haven't started on this but if you have we should go with what you have and get it merged in before #1470 -- I have some changes that I can test through that process to help verify this new PR we are discussing here. The key thing is that we don't want to duplicate and/or waste effort.

Do you mind sharing a branch and/or give me some idea of timeline? I was hoping to finish off #1470 today (or in the worst case tomorrow). I have had some changes to the scripts but they are relatively minor compared with the entire refractor for formatting so we could also potentially retroactively apply my changes to yours via a rebase too.

xylar commented 6 years ago

@pwolfram, I'm in the middle of a train ride with 4 1/2 hours to go. I should be able to take care of this today and create a PR in a couple of hours (maybe sooner).

Still, maybe I'm missing something about #1470 but it seems like the PR I'm working on is almost completely orthogonal to that one unless you have some unmerged changes to the COMPASS scripts that I haven't seen. Maybe it makes sense to continue with #1470 and have me make the (minor) fixes to my upcoming PR once that's merged.

pwolfram commented 6 years ago

Ok, I think that plan (merge #1470 with minor script changes) and then merge your PR makes the most sense. I'll just focus on #1470 now (with its fairly minor script changes). Thanks!

xylar commented 6 years ago

I'll make sure it passes testing via the linter, spyder, and modelines to be as general as possible

The MPAS linter only supports Fortran, C and XML files. Ironically, if we were to add linting of python files to the linter itself probably wouldn't pass...

I'm not really set up to check modelines so I'll need to leave that to you as part of your review.

pwolfram commented 6 years ago

Sounds good, I was thinking pep8 for the "linter", sorry about the confusion. I'll verify modelines work on my side. Thanks!

pwolfram commented 6 years ago

Closed by #1474

pwolfram commented 6 years ago

Thanks @xylar!