Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 96.46% 96.48% +0.02%
==========================================
Files 15 15
Lines 509 512 +3
==========================================
+ Hits 491 494 +3
Misses 18 18 ☔ View full report in Codecov by Sentry. |
moradology
left a comment
moradology
left a comment
There was a problem hiding this comment.
I have one minor gripe but it is ultimately aesthetic. Feel free to just hit 'ready for review' again if you think I'm wrong!
| target_cls_args_str = ", ".join( | ||
| f"{k}={v}" for k, v in self.pangeo_forge_target_class_args.items() | ||
| ) | ||
| return base + (f", {target_cls_args_str}" if target_cls_args_str else "") + ")" |
There was a problem hiding this comment.
I think this should be equivalent and a bit terser/easier to read: f"{base}, {target_cls_args_str})"
(I think join here should produce "" if no iterations happen from items(), so the check is not necessary)
| self.fsspec_class(**self.fsspec_args), | ||
| root_path=self.root_path.format(job_name=job_name), | ||
| fsspec_kwargs=self.fsspec_args, | ||
| **self.pangeo_forge_target_class_args, |
There was a problem hiding this comment.
Might be good to disambiguate the intents of pangeo_forge_target_class_args and fsspec_args
for more information, see https://pre-commit.ci
2 participants
pangeo-forge/pangeo-forge-recipes#630 introduces the possibility that storage classes could take args.
This PR adds the ability to passthrough such args.