Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

albu-diku
Copy link

No description provided.

Copy link
Contributor

@jonasbardino jonasbardino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good 👍

Please add the mandatory copyright notice and module level docstring in all new code files as previously mentioned. The header of mig/shared/base.py can be used as a template or you can rely on the addheader.py helper.

@jonasbardino jonasbardino self-assigned this Jun 13, 2024
@jonasbardino jonasbardino added the unit test Code and infrastructure related to unit testing label Jun 13, 2024
@jonasbardino
Copy link
Contributor

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.

tests/support.py Outdated Show resolved Hide resolved
@albu-diku albu-diku changed the base branch from addition/envhelp-pythonpath to edge June 17, 2024 11:24
@jonasbardino
Copy link
Contributor

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 albu-diku force-pushed the test/mig.shared.localfile branch 2 times, most recently from 1d45c36 to f327fb7 Compare June 25, 2024 11:46
@albu-diku
Copy link
Author

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.

Copy link
Contributor

@jonasbardino jonasbardino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jonasbardino
Copy link
Contributor

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit test Code and infrastructure related to unit testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants