ENH: add helpers to merge extra data into a cycler instance#47
ENH: add helpers to merge extra data into a cycler instance#47tacaswell wants to merge 2 commits intomatplotlib:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
==========================================
- Coverage 100% 98.41% -1.59%
==========================================
Files 1 1
Lines 184 189 +5
Branches 51 53 +2
==========================================
+ Hits 184 186 +2
- Misses 0 3 +3
Continue to review full report at Codecov.
|
From private discussions with @WeatherGod
tacaswell
force-pushed
the
enh_suplemental_data
branch
from
68e832a to
5f00bec
Compare
|
@WeatherGod took 18 months, but I addressed your comments! |
WeatherGod
left a comment
WeatherGod
left a comment
There was a problem hiding this comment.
Should also have unit test, which could just be duplicated for the docstring example I mentioned, too.
|
|
||
| Parameters | ||
| ---------- | ||
| inp : Iterable[Mapping[Any, Any]] |
There was a problem hiding this comment.
to be pedantic, wouldn't the first Any be Hashable? I haven't gotten fully into the annotation habit yet, so I don't know if Mapping already implies Hashable for the keys.
| """Update a cycler with some supplemental data | ||
|
|
||
| Given a cycler, add extra keys to each entry based | ||
| on the value of ``index_key`` in that entry. |
There was a problem hiding this comment.
name here doesn't match the name in the call signature.
| A mapping between the values of ``index_key`` in ``source`` | ||
| and mappings of additional keys and values. | ||
|
|
||
| Each mapping must have the same set of keys. |
There was a problem hiding this comment.
This docstring would greatly benefit from an example. As it stands right now, I have to think really hard to understand the use-cases for this function (and I am the one who originally thought of this idea!).
|
Is there a reason these are stand alone functions? I would have expected
|
|
@timhoffm Fair, but I think that one level of nesting Still not sure this is a good idea. Maybe this should just go into the docs as an example? |
4 participants
From private discussions with @WeatherGod
This still needs: