added subcommand to clean deleted relations for issue # 2444 #3224
Conversation
That's the most recent version of mypy now correctly detecting a missing type declaration. I've just fixed this on master. Can you please rebase your change and force-push the result to your branch? |
lujoh
force-pushed
the
clean-deleted-relations
branch
from
f3c0cd1
to
e9efef9
Compare
| @@ -41,6 +41,8 @@ def add_args(self, parser: argparse.ArgumentParser) -> None: | |||
| help='Print performance analysis of the indexing process') | |||
| objs.add_argument('--collect-os-info', action="store_true", | |||
| help="Generate a report about the host system information") | |||
| objs.add_argument('--clean-deleted', action='store_true', | |||
| help='Clean up deleted relations') | |||
There was a problem hiding this comment.
If you want you can simply make the age a required argument to --clean-deleted. Then argparse ensures that the the argument is present and the code can be a lot simpler.
There was a problem hiding this comment.
Thank you for your feedback @lonvia! I wasn't able to make the age required directly because it was causing errors with the other commands that didn't include age. Instead I decided to raise a parser error right in the clicmd/admin.py file:
if not args.age: self.parser.error('Age is required for --clean-deleted command')
I moved the test for that into the cli/test_cmd_admin.py file. I think the solution is a bit cleaner, but if you know a way to make the argument required directly I'd be happy to switch to that instead.
There was a problem hiding this comment.
I meant to give clean-deleted an argument:
objs.add_argument('--clean-deleted', action='store', metavar='AGE', help='...')
...
age = args.clean_deleted
...
But the current solution works as well. Just one tiny nitpick: simply do raise UsageError() instead of using self.parser.error(). Just for consistency with the rest of the code.
There was a problem hiding this comment.
Thank you for clarifying lonvia. I misunderstood where you were going with that. I like your solution better. I've made the required changes for your comments.
test/python/tools/test_admin.py
Outdated
| END; | ||
| $$ | ||
| LANGUAGE plpgsql; | ||
| """) |
There was a problem hiding this comment.
Can you try out if it is possible to use the original code here, so that we catch in the unit tests, when something breaks in the SQL? The following should work:
- move
flush_deleted_places()fromlib-sql/functions/place_triggers.sqlin tolib-sql/functions/utils.sql - load the original
utils.sqlhere similar as it is done in https://github.com/osm-search/Nominatim/blob/master/test/python/tokenizer/test_icu.py#L100
There was a problem hiding this comment.
I moved the function over and it seems to be working fine. I had to add a couple of extra tables and functions to the setup for the unit tests to cover everything in utils.sql.
test/python/tools/test_admin.py
Outdated
| temp_db_cursor.execute("""INSERT INTO placex (place_id, osm_id, osm_type, class, type, indexed_date, indexed_status) | ||
| VALUES(1, 100, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), | ||
| (2, 145, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), | ||
| (3, 175, 'R', 'landcover', 'grass', current_date - INTERVAL '1 month', 1)""") |
There was a problem hiding this comment.
Give them different ages and then test that according to the age parameter really only a part of them is deleted.
|
Thank you! |
This adds the subcommand to clean deleted relations.
Fixes #2444
The pull request passes local test, but fails the GitHub Actions with this message:
nominatim/api/v1/helpers.py:52: error: Returning Any from function declared to return "tuple[int, int]" [no-any-return]This error doesn't seem to be related to the topic of the pull request.
Please let me know how to proceed and if I need to make any changes.