Wkcuber with typer #903
Wkcuber with typer #903
Conversation
There was a problem hiding this comment.
Good stuff. It is looking nice already. Good to see all the code gone.
I think we should keep some of the old modules, though, because users are still using these.
Also, please check that the Zarr support still works (including remote stores such as S3, HTTP). This is very important for us in the future because we want to drop WKW in favor of Zarr.
There was a problem hiding this comment.
Good job integrating Typer and simplifying the wkcuber into a light-weight wrapper. This was long overdue.
Please see my comments for improvements. I was very picky with the naming conventions and help texts but these are the most user-facing "interface" when using the CLI tools. They should be consistent, as precise and helpful as possible. Offer small examples wherever possible, especially for SCM-proprietary formats, e.g., Mag strings.
If we do not explain our tools well, people will find them frustrating or difficult and not use them, which would be a waste. #make our users happy
There was a problem hiding this comment.
Good job integrating Typer and simplifying the wkcuber into a light-weight wrapper. This was long overdue.
Please see my comments for improvements. I was very picky with the naming conventions and help texts but these are the most user-facing "interface" when using the CLI tools. They should be consistent, as precise and helpful as possible. Offer small examples wherever possible, especially for SCM-proprietary formats, e.g., Mag strings.
If we do not explain our tools well, people will find them frustrating or difficult and not use them, which would be a waste. #make our users happy
Co-authored-by: Tom Herold <tom.herold@scalableminds.com>
Co-authored-by: Tom Herold <tom.herold@scalableminds.com>
…sos-libs into wkcuber_with_typer
wkcuber/Changelog.md
Outdated
There was a problem hiding this comment.
Careful with the autoformatter here. The automatic release tools require the empty lines.
|
@markbader I had a quick look through the PR changes. Many comments have already been addressed. Nice! Please mark resolved comments with the GitHub feature/button "Resolve Conversation". There are a number of open questions from my original PR feedback which remain unanswered. Please respond (alternatively, we can go through them offline). |
webknossos/tests/test_cli.py
Outdated
| TESTDATA_DIR = Path(__file__).parent.parent / "testdata" | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module", name="remote_testoutput_path") |
There was a problem hiding this comment.
Could we share this code with test_dataset.py?
…sos-libs into wkcuber_with_typer
…sos-libs into wkcuber_with_typer
Co-authored-by: Norman Rzepka <code@normanrz.com>
webknossos/tests/test_cli.py
Outdated
| str(tmp_path), | ||
| ], | ||
| ) | ||
| assert result.exit_code == 0 |
There was a problem hiding this comment.
I think for differing datasets, the exit code should not be 0. Otherwise it is hard to use this command in bash scripts.
|
I did another round of testing. Once the exit code in |
|
Great stuff! Thanks for pushing through on this. I'll leave you the honor of pressing the merge button! |
markbader
dismissed
hotzenklotz’s stale review
The requested changes are implemented and @normanrz approved this PR
|
So great to see this happening! 🎉 Congrats @markbader! |
Description:
Issues:
Todos:
Make sure to delete unnecessary points or to check all before merging: