enh: implement metadata aggregator for asset summary #34
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 94.65% 95.75% +1.09%
==========================================
Files 11 11
Lines 879 965 +86
==========================================
+ Hits 832 924 +92
+ Misses 47 41 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
satra
added
patch
IMHO aggregate is not sufficiently descriptive of it purpose and behavior (mutates, instead of return aggregated value). AFAIK this function is not (yet at least) to be used by external tools, so I made it protected. Also added some type hints while at it
It is used only once, not generic (spreads logic about variableMeasured). Placing that logic directly where used IMHO makes it easier to follow the code
That is AFAIK where all models are defined and IMHO they should not be in metadata, which is supporting functionality for dealing with all those models/metadata
so we somehow do not match a directory or some funkily named non-nii file
I do not quite like aggregate_assets_summary and would prefer to start producing constructors such as AssetsSummary.from_metadata and alike. But I disliked toSummary more: name camel cased, mutating provided stats. Notable other difference - decided to return AssetsSummary itself, and let outside code to do .json_dict() or any other desired serializer
Made them all private since we do not / want to expose them AFAIK since otherwise would become a part of the interface etc
Detected by mypy and I think it would be a legit thing to do
to keep metadata module interface free of manipulating models. Later we might RF to get AssetsSummary.from_assets and alike
Explicit better than implicit and IMHO semantically (since we are not saying that it is ALLOWED_OLDER_INPUT_SCHEMAS) I think it would be more correct. Did it in the code to avoid hardcoding duplicates
* upstream/master: Update dandischema/datacite.py Only attempt to remove contributorType if present
|
Let's proceed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR implements the computation of asset summary for a dandiset to this library.
addresses part 1 of dandi/dandi-archive#337
will file a PR on API to use this.