Repository navigation
Add code duplication analysis to the dev workflow - #1944
Draft
yarikoptic wants to merge 25 commits into
Draft
yarikoptic wants to merge 25 commits into
yarikoptic wants to merge 25 commits into
Conversation
Wire jscpd (pinned to 5.3.3) in as a `tox -e duplication` env, backed by tools/check-duplication.sh. Config lives in .jscpd.json, scanning dandi/ and tools/ with sensible ignores for vendored/generated files, test fixture data, and build artifacts. Threshold is temporarily set to 5% (above the measured 4.38% baseline) so this lands green; a follow-up commit tightens it after mitigating the duplication found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add .github/workflows/duplication.yml, mirroring the existing typing.yml/lint.yml pattern: a dedicated job that installs tox and runs `tox -e duplication`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The private _bids_validate() implementation repeated the full Parameters/Returns docstring of its public wrapper bids_validate(). Since _bids_validate is internal-only, point its docstring at the public function instead of duplicating the parameter docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Its docstring repeated flatten_meta_to_pyout()'s prose almost verbatim; point it at the sibling function and describe only how it differs (recursive flattening of nested dicts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
get_assets, get_assets_with_path_prefix, and get_assets_by_glob shared the same paginate/except/yield body, differing only in the request params; extract it into _iter_version_assets(). upload_raw_asset and iter_upload_raw_asset shared the same dandi_file()/isinstance check for resolving a LocalAsset; extract it into _local_asset_file(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BaseRemoteAsset.get_download_file_iter and RemoteZarrEntry.get_download_file_iter shared the same GET-with-Range-header/raise_for_status logic; extract it into module-level _get_download_response(). RemoteBlobAsset.set_raw_metadata and RemoteZarrAsset.set_raw_metadata were identical apart from the blob_id/zarr_id key; extract the shared PUT + in-place field update into RemoteAsset._put_raw_metadata(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ethods LocalAsset.upload, the abstract LocalAsset.iter_upload, LocalFileAsset's concrete override, ZarrAsset's override, and the deprecated DandiAPIClient.iter_upload_raw_asset all repeated the same Parameters/Returns prose. Keep the full docs on upload() (the one method every subclass inherits unchanged) and have the others reference it, keeping only what's actually specific to each override. NWBBIDSAsset.get_validation_errors and ZarrBIDSAsset.get_validation_errors had identical bodies apart from which sibling class's method they combined with BIDSAsset's; extract _bids_combined_validation_errors(). ZarrBIDSAsset.get_metadata duplicated BIDSAsset.get_metadata's body plus one extra line; have it delegate instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RemoteZarrEntry deliberately stopped inheriting from `misctypes.BasePath` (0.48.0), but its suffix/suffixes/stem/match implementations were copy-pasted from BasePath verbatim. Extract the underlying logic into module-level helpers in misctypes.py (_path_suffix, _path_suffixes, _path_stem, _match_parts, _split_path) that both classes delegate to, without re-establishing the inheritance that was intentionally removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AssetPathPrefixURL, AssetFolderURL, and AssetGlobURL's get_assets() shared the same "look up the dandiset, iterate assets, track whether any were found, raise NotFoundError if strict and none were" body, differing only in the query method called and the error message. Extract it into _iter_multi_assets(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
calculate_moves, calculate_moves_by_regex, get_assets, get_path, and move are declared abstractly on Mover/LocalizedMover with a full docstring, then re-implemented with the same docstring copy-pasted verbatim in LocalMover, RemoteMover, and (for the calculate_moves* pair) LocalRemoteMover. Keep the docs on the abstract declarations and have every concrete override point back at them instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
simple4_nwb/simple5_nwb built near-identical NWBFile+TimeSeries+Subject structures differing only in the TimeSeries unit and subject_id; extract _make_ambiguous_orientation_nwb(). organized_nwb_dir/organized_nwb_dir3 both ran `organize -f copy` on a given NWB fixture into a fresh dandiset dir; extract _organize_copy(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every test function repeated the same three lines (snapshot starting assets, monkeypatch.chdir to some directory, monkeypatch the API key env var). Extract _setup_move() and call it from all 34 test functions; the chdir target still varies per test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Merge test_move_skip/test_move_overwrite into test_move_existing, parametrized on the existing= value and expected remapping. - Merge test_move_pyout/test_move_pyout_dry_run into one test parametrized on dry_run and the expected remapping. - Merge test_move_file_slash_src/test_move_file_slash_dest into test_move_file_slash, parametrized on which path (src or dest) gets the trailing slash. Remaining near-identical pairs in this file (dandiset-by-path vs dandiset-by-URL addressing, and the AssetMismatchError scenarios under work_on=BOTH) test genuinely different setups/expected messages; merging them would obscure what each case actually asserts, so they're left as intrinsic test duplication. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- test_download_sync/test_download_sync_do: extract the shared "delete file.txt, move dandiset dir into tmp_path, patch abbrev_prompt" setup into _prep_sync_download(). - test_download_sync_folder/test_download_sync_glob: extract the shared "delete two assets, patch abbrev_prompt" setup into _prep_sync_delete_two_assets(). - test_download_glob_option/test_download_glob_url: extract the shared post-download assertions into _assert_glob_download(). - test__check_attempts_and_sleep_retries: two inline scenarios only differed in the Retry-After/now timestamps and whether a sleep was expected; factor into a local check_retry_after() helper. The remaining duplication in this file is a large existing @pytest.mark.parametrize data table for progress-event aggregation (intrinsic to the parametrize pattern itself, not code duplication) plus one small residual signature/call overlap between two tests that exercise genuinely different URL addressing schemes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- test_get_content_url/test_get_content_url_regex: extract the shared "stream the content URL to asset.nwb" tail into _fetch_content(). - test_get_dandiset_lazy/non_lazy/no_version_id/published/ published_no_version_id/published_draft/published_other_version: extract the shared post-fetch field assertions (version_id, created, modified, version, most_recent_published_version, draft_version, contact_person -- including dropping an accidentally double-asserted `isinstance(dandiset.created, datetime)` line) into _assert_dandiset_fields(). The remaining duplication is mock-setup scaffolding across the test_authenticate_* variants, each exercising a genuinely different keyring/input code path, plus small residual call-site overlap between tests whose only real content is a differing (version_id, most_recent_published) pair. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The 10 test_get_nonexistent_* tests all parsed a URL, asserted get_assets() returns empty, and asserted strict=True raises a NotFoundError with a specific message; extract this into _assert_no_assets(). Also factor the repeated "No such Dandiset: '999999'..." message into NO_SUCH_DANDISET_999999. test_utils.py: test_get_instance_dandi_with_api and test_get_instance_url mocked an identical server-info JSON payload; factor it into the SERVER_INFO constant. The remaining cross-file overlap between test_dandiarchive.py and test_utils.py is two mock server-info payloads that happen to share structural JSON keys but carry different actual data (different version/services) -- coincidental, not real duplication. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- The 5 test_session_duration_with_* tests shared the same "session_start_time/session_end_time present, duration within 1s of expected" check; extract _assert_session_duration(). Two of them additionally shared the same "Session activity has matching start/end dates" check; extract _assert_session_activity_dates(). - test_nwb2asset and test_nwb2asset_remote_asset asserted against the same large BareAsset.model_construct(...) golden object, differing only in contentSize/digest/path/blobDateModified/strain; extract _expected_simple2_asset(). The remaining duplication is the metadata2asset @pytest.mark.parametrize data table (literal test-fixture dicts covering distinct corner cases, explicitly labeled as such in a comment) -- intrinsic to the parametrize pattern, not code duplication. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every test asserted dandi.download.download() was called with the
same 10-kwarg set (existing/format/jobs/.../path_type), varying only
one or two values. Factor the defaults into DEFAULT_DOWNLOAD_KWARGS
and assert with `**{**DEFAULT_DOWNLOAD_KWARGS, "some_key": override}`.
test_download_gui_instance_in_dandiset and
test_download_api_instance_in_dandiset additionally shared their
whole setup/invoke/assert body apart from the instance name and
expected URL; extract _invoke_in_dandiset().
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Most tests repeated "monkeypatch the API key env var, grab instance_id, spy on RESTFullAPIClient.delete"; extract _setup_delete_spy(), taking either a text_dandiset.api or local_dandi_api (both DandiAPI instances). The skip_missing variants additionally repeated a "download and compare remaining asset paths" tail; extract _assert_remaining_assets(). The 3 remaining pairs (single-path vs whole-dandiset delete; path-confirm vs dandiset-confirm; missing-asset vs missing-folder) only share the mechanical call shape around genuinely different scenarios and assertions, so they're left as intrinsic duplication. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test_find_dandi_files asserted three overlapping find_dandi_files() result lists that repeated the same VideoAsset/ImageAsset/ZarrAsset/ NWBAsset/DandisetMetadataFile literals; factor them into local variables reused across the three assertions. test_find_dandi_files_with_bids similarly repeated each BIDS asset literal between the top-level files list and the per-dataset dataset_files checks; factor into local variables (bidsignore, bids1_dd, bids1_file, ..., bids2_subdd). Equality is value-based (dataclasses), so reusing the same instance across multiple assertions is safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6 tests repeated "delenv PYTHON_KEYRING_BACKEND, patch dandi.keyring_utils.get_keyring" with only the returned backend instance varying; extract _mock_no_keyring(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- test_upload_different_zarr/test_upload_loose_zarr shared the "replace the local zarr, re-upload, verify same zarr id, download and compare" tail; extract _replace_zarr_and_check(). - test_upload_rejects_dandidownload_paths's "Test 1" sub-case and test_upload_rejects_dandidownload_nwb_file both built an NWB file and expected upload() to reject it with the same message; extract _assert_upload_rejects_dandidownload(). - test_upload_zarr_patch_mode_no_delete/test_upload_zarr_full_mode_delete shared the "find and remove a local zarr data file" setup; extract _remove_a_zarr_data_file(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test_render_text_grouping and test_render_text_multilevel_grouping built the same two-issue ValidationResult list; extract _sample_validation_issues() and have the multilevel test append its extra ERROR-severity issue on top. The remaining test_cmd_validate.py/test_core.py overlap (test_validate_bids_error vs test_validate_bids_errors) exercises the same BIDS error-example scenario through two different entry points (the CLI command vs the underlying validate() function) with different assertions; left as intrinsic duplication across module/layer boundaries rather than merging two different-layer tests together. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Baseline was 4.38% (133 clones) across dandi/ + tools/. After the mitigation commits on this branch, duplication is down to 1.46% (42 clones), all now residual and documented: - Intrinsic @pytest.mark.parametrize data tables (test_download.py's progress-event table, test_metadata.py's metadata2asset corner cases) -- literal fixture data, not code duplication. - Intentional parallel-layer test coverage (public bids_validate() vs private _bids_validate(); CLI `validate` command vs core validate() function; single-path vs whole-dandiset delete scenarios) where merging would conflate independent test layers. - Python-level unavoidable boilerplate (method-override signatures repeating parameter lists; Sphinx `.. versionchanged::` directives that must appear on each documented method). - One deliberately-decoupled duck-typing case (RemoteZarrEntry explicitly stopped inheriting misctypes.BasePath in 0.48.0; the residual is just per-class property-delegation boilerplate). Threshold set to 2% (just above the current 1.46%) rather than 0% to leave headroom for this residual without treating it as a bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
No pre-commit hook for this? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from dandi.bids_validator_deno import bids_validate | ||
|
|
||
| from .bases import GenericAsset, LocalFileAsset, NWBAsset | ||
| from .bases import DandiFile, GenericAsset, LocalFileAsset, NWBAsset |
Comment on lines
248
to
-254
| def calculate_moves( | ||
| self, *srcs: str, dest: str, existing: MoveExisting | ||
| ) -> list[Movement]: | ||
| """ | ||
| Given a sequence of input source paths and a destination path, return a | ||
| sorted list of all assets that will be moved/renamed | ||
| """ |
Contributor
There was a problem hiding this comment.
IDK about this though; sometimes these docstrings are not for some user asking ? on a function, but instead a dev looking at that code as text on the page, so adding redirection is detrimental to that
I can understand deduplicating code / centralizing utils, which with appropriate function names can make things easier to read
Member
Author
There was a problem hiding this comment.
yeah, also not sure on this yet... I kinda hate duplication there, especially in heavy API'ed adapters etc, where then there is a page of docstring for 1-2 lines of code; but also hate chasing the rabbit down the chains of redirects. IIRC in PyMVPA we had even @borrowdoc decorator helper to simply copy the docstring from another class etc ;-)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1944 +/- ##
==========================================
- Coverage 78.34% 78.14% -0.20%
==========================================
Files 92 92
Lines 14109 13853 -256
==========================================
- Hits 11053 10825 -228
+ Misses 3056 3028 -28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| def __truediv__(self: P, path: str) -> P: | ||
| p = self | ||
| for q in self._split_path(path): |
Contributor
There was a problem hiding this comment.
Well, I understand they are private methods (static or no) and so could change on a whim, but this does represent actual core changes to the API so hopefully we don't break anything externally
Member
Author
I thought it takes awhile... but seems fast here ... so indeed, should likely also be "skilled" to be added, and depending on either pre-commit.ci used or dedicated CI run, create or not the github workflow |
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add code duplication analysis (jscpd) to the development workflow, and eliminate most of the duplication it found.
Changes
.jscpd.json(pinned to jscpd 5.3.3), scanningdandi/+tools/, ignoring vendored/generated files (_version.py,versioneer.py), test fixture data, and build artifacts.tox -e duplicationcheck backed bytools/check-duplication.sh..github/workflows/duplication.yml), mirroring the existinglint.yml/typing.ymlpattern.dandiapi.py,misctypes.py,dandiarchive.py,move.py,files/bases.py/bids.py/zarr.py,cli/cmd_ls.py,bids_validator_deno/_validator.py) and test files (test_move.py,test_download.py,test_dandiapi.py,test_dandiarchive.py,test_metadata.py,test_delete.py,test_files.py,test_keyring.py,test_upload.py,test_cmd_validate.py,tests/fixtures.py), via extracted helpers/shared functions and@pytest.mark.parametrize/shared fixtures.RemoteZarrEntryvsBasePath) where merging would re-couple classes that were deliberately decoupled in 0.48.0.Testing
✅
tox -e duplicationpasses at the 2% threshold (and was verified to actually fail when set below the measured baseline)✅
mypy dandiandflake8 dandi setup.py— clean✅ Full non-network test suite (
pytest -m "not obolibrary" dandi): 843 passed, same 47 pre-existing failures as onmaster(missingbids-validator-denobinary in this sandbox — unrelated to this change), no regressions🤖 Generated with Claude Code