search

ucphhpc / migrid-sync

MiGrid workspace where master branch is kept strictly in sync with SF upstream svn repo. Any development or experiments should use a branch. You probably want to fork your own clone or work e.g. on the edge branch if you wish to contribute.
GNU General Public License v2.0
3 stars 3 forks source link

Put together LocalFile test primarily to provide a file lock assertion. #66

Closed albu-diku closed 2 weeks ago

jonasbardino commented 3 weeks ago

For the record I've added a new unit test label and think we should use it on these and aim for having a fundamental set of such unit tests in place for the "Python3 support for sensitive data sites" milestone.

jonasbardino commented 2 weeks ago

Did you see my initial comment about missing copyright header? Not sure if you intended to push that fix before re-requesting review, but I think we can merge once that is addressed.

albu-diku commented 2 weeks ago

Did you see my initial comment about missing copyright header? Not sure if you intended to push that fix before re-requesting review, but I think we can merge once that is addressed.

I thought I had addressed that but guess I fell into the blindly rebasing forward trap :/ should be fixed as of the last force push.

jonasbardino commented 2 weeks ago

Thanks. Manually merged through svn fixing the above stat.ISDIR call to stat.S_ISDIR and with minor polish in line with review comments.

albu-diku commented 2 weeks ago

Thanks, just a few of minor comments: Functions/methods should generally have at least brief docstrings. These are quite simple functions and I understand it's tempting to call them self-explanatory but while e.g. fixturepath might be obvious to you now, that's not necessarily the case to somebody else (including future you :-) reading the code later. When used consistently docstrings also come in handy for various automatic code doc generators.

The 5th line of the copyright header addheader - BLA should reflect the module name and a one-liner about what it does.

Please use single letter variables like f sparingly, except e.g. for simple iterations. It's an absolute pain to identify occurrences across the code later (been there, done that, absolutely hated it). While some IDEs can to some extent handle them in search & refactoring we do not all rely on such and especially not when working with remote dev/test systems. I suggest we include a line or two about such considerations in the coding style write-up we previously talked about.

I'll merge and address a few such things this time, so please take it mainly as hints for future development.

Responded to these points in private messages, but for the sake of record: as regards this PR a couple of these shortcomings are due to this being a many weeks old PR that I rebased forward somewhat blindly, but suffice to say I will take all these suggestions on board and will work to incorporate these into any work going forward.