HDXDSYS-467 Refactor create revise #60
Conversation
|
I had a look at the slightly reduced coverage but don't propose to try to add the 0.2% back again as it looks like the important code is covered and it could be due to reducing the amount of code so that as a percentage of the total anything that continues missing coverage gets larger. |
There was a problem hiding this comment.
Looks good to me!
I think my comments are all around explanation rather than actual code.
_prepare_hdx_call could do with some documentation since it seems to be new
_revise_dataset seems to be where all the action is - it is a rather large chunk of code which could do with splitting up/commenting - maybe extract functions at lines 644 and 682
The massive lump of data in test_update_dataset_resources.py at line 101 could go into a JSON format fixture file which would make the test a bit more readable.
I would comment on the shift in language from merge_update to update - I think it is a good thing.
A few "process" comments based on my previous experience:
- I use a PR template which has sections on the purpose of the change, a summary of major file changes, minor file changes and a check for bumping the version. (example here: https://github.com/OCHA-DAP/hdx-cli-toolkit/blob/main/PULL_REQUEST_TEMPLATE.md)
- Commit messages could have been a bit more informative!
- The list() -> [] and dict() -> {} are good but maybe should have gone into their own commit - I assume you did them with search and replace. As I understand [] and {} are faster than list() and dict().
- The test coverage looks really good - personally I would look to tuning the coverage test so that it passes because coverage exceeds a threshold rather than failing because of a trivial reduction in test coverage. Merging a PR with failing test makes me sad :-(
- I normally raise an issue (or issues) on GitHub which describes the problem the PR is intended to fix
Move code in _revise_dataset into 2 functions _revise_filter and _revise_files_to_upload_resource_deletions Move test data for test_update_dataset_resources into fixture
… so that the former is always first Improve documentation of _dataset_update_resources and remove mention of merging data sicne we are not doing that any more - we are comparing.
|
Thanks @IanHopkinson for your helpful feedback. I admit I'm sometimes a bit lazy with commit messages and also in not doing separate commits (or even PRs) for changes like the dict -> {} unrelated to the main purpose of the PR :-( Ironically adding all this has improved test coverage by virtue of there being more lines :-) |
|
@mcarans that looks good! Some of these things I aspire to do but am not necessarily consistent in actually doing I once parsed Scala source code with regular expressions to extract data, so I'm sensitive to seeing big blobs of data in code files - if it is extracted out into separate data files it becomes reusable. |
|
I've been doing some testing on stage (editing datasets, adding, removing, and reordering resources, adding and removing tags) and everything is behaving the way I expect it to! |
I've done a major refactor of HDX Python API's create and update code. It fixes the issue found by HOTOSM where tags that were deleted did not get removed from the dataset. The old logic just put into the update field passed to package_revise the merge of the dataset dictionary read from HDX with the metadata to be updated. This was done to minimise changing the code from when package_update was used. However, I misunderstood how the update field operates - elements in lists are not removed by changes in the update field.
The new logic follows the package_revise approach fully, putting only the metadata to be updated in the update field and working out what needs to be deleted to put into the filter field.