grass.app: Move mapset locking to library #4158
Conversation
This moves the lock_mapset function to the library. It introduces only small changes to the code, i.e., proper refactoring and deduplication in relation to the code elsewhere is still needed. However, this unifies the error handling to always raising a custom exception. It also fixes the reported user using the mapset (before always the current user, now, the owner of the lock file). It removes the debug message which I don't find particularly useful, for example it is currently misleading on windows and it is relatively easy to confirm the actual existence in process manager when debugging. Committing with --no-verify due to textproto issue with pre-commit.
…format function everywhere. Add missing functions.
…needs caller change).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This is now ready for review. Description updated accordingly. |
|
What's the testing strategy you'd want here? For now, these code paths don't seem used by any pytest test, so we don't have code coverage to show that the new code is used (and the old one was too). I have Python code coverage working properly for gunittest on a PR in my fork, but it almost doubles the testing time, so I'm not pushing for it now (Ubuntu and macOS). |
|
Basic tests are done by just running the The situation is not ideal. There is at least one bug in the original code which I carried over. My hope is that with more refactoring this will be easier to fit and easier to test. I welcome suggestions on getting the executable path or writing the tests for these in general, but it seems to me that in this phase, we need to let these PRs pass without much automated testing. |
I'm fine with this approach, we need to incrementally do better and not stick too long for PRs that improves and makes the repo in better shape than without. So what remains to review then is the actual Python. In my quick screening yesterday I didn't find anything that popped to my eyes. I invite another Python-only review. |
|
Merging main to have it check on the pytest on macOS to see if the importing still works (it should, but if it fails there because of spawn it would have failed on windows too) |
It didn't fail :) |
| msg = _( | ||
| "Unable to properly access '{}'.\nPlease notify your system administrator." | ||
| ).format(lockfile) |
There was a problem hiding this comment.
Follow up from #4158 (comment)
| msg = _( | |
| "Unable to properly access '{}'.\nPlease notify your system administrator." | |
| ).format(lockfile) | |
| msg = _( | |
| "Unable to properly access '{lockfile_path}'.\nPlease notify your system administrator." | |
| ).format(lockfile_path=lockfile) |
@veroandreo Does this make more sense than without it? I'm not completely sure, and it is the last element remaining for this PR to be unblocked.
(It might need to be reformatted, did it free-handed)
This moves the lock_mapset function to the library. It introduces only small changes to the code, i.e., proper refactoring and deduplication in relation to the code elsewhere is still needed.
However, this unifies the error handling to always raising a custom exception. It also fixes the reported user using the mapset (before always the current user, now, the owner of the lock file; for that, lock file is removed only after getting its owner). It removes the debug message which I don't find particularly useful, for example it is currently misleading on windows and it is relatively easy to confirm the actual existence in process manager when debugging.