Ruggedizes list_local_datasets and adds basic tests for CLI #126
Conversation
There was a problem hiding this comment.
A few comments and questions. The ruggedization of list_local_datasets all seems correct; the pattern for testing the CLI seems correct, and the only potential improvement would be mocking functions such as download_dataset, though I don't know what the pattern is for doing that, and I don't think it's essential.
| def test_list_app(): | ||
| result = runner.invoke(app, ["list", "local", "--all"]) | ||
| assert result.exit_code == 0 | ||
| # some of the other columns may be cut off by Rich |
There was a problem hiding this comment.
what is "Rich" as alluded to in this comment is that rich text?
There was a problem hiding this comment.
Line 8 in da8578c
my initial plan was just check if the whole header is what is expected in the table but the table format changes depending on terminal size. i'm pretty sure there is a way it could be done by specifying something related in rich but i think it might require changing more aspects of the cli.py file
There was a problem hiding this comment.
I think it's fine as-is, I didn't realize rich as actually a package we used in the CLI
| """ | ||
|
|
||
| # might have to clear up the local dataset first. | ||
| # ideally this seems like it could just be handled by the tests |
There was a problem hiding this comment.
It might be possible to mock the actual download_dataset function called by the cli function if we really want to avoid repetition, particularly because dataset downloads are slow and add to the overall test time, but I don't think it's strictly speaking required.
There was a problem hiding this comment.
these lines + 34+35 probably can be removed but i dont know if some other test will download the dataset for testing elsewhere and this runs concurrent or afterwords and the dataset wasnt cleaned up by the other test.
There was a problem hiding this comment.
As long as each test cleans up after itself, its fine if they also have redundant cleaning at the beginning. Your test cleans up the dataset it downloads, so I think it's fine.
| @@ -36,16 +36,19 @@ def _show_dataset_table(datasets, table_title): | |||
| table.add_column("Email", justify="left", style="magenta") | |||
|
|
|||
| for dst_metadata in datasets.values(): | |||
| author = dst_metadata.get("author", "Unknown") | |||
There was a problem hiding this comment.
Seems like a reasonable ruggedization.
| @@ -59,8 +59,9 @@ def list_local_datasets( | |||
| main_file_path = os.path.join(datasets_path, dst_id, "data/main_data.hdf5") | |||
| with h5py.File(main_file_path, "r") as f: | |||
| metadata = dict(f.attrs.items()) | |||
| if compatible_minari_version and __version__ not in SpecifierSet( | |||
| metadata["minari_version"] | |||
| if ("minari_version" not in metadata) or ( | |||
There was a problem hiding this comment.
Also good ruggedization
|
the pre-commit hook let me commit because its showing issues in other files not related to this, is running with |
balisujohn
changed the title
Description
There was an issue where if you used minari cli it would break if you had datasets that metadata isnt available. Can see here:
Fixes # (issue), Depends on # (pull request)
Type of change
Screenshots
Checklist:
pre-commitchecks withpre-commit run --all-files(seeCONTRIBUTING.mdinstructions to set it up)pytest -vand no errors are present.pytest -vhas generated that are related to my code to the best of my knowledge.