-
Notifications
You must be signed in to change notification settings - Fork 18
Ensure consistent download_dataset
for Copernicus
data
#522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The problem with |
Metadata
download_dataset
for Copernicus
download_dataset
for Copernicus
download_dataset
for Copernicus
data
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #522 +/- ##
==========================================
- Coverage 14.54% 14.51% -0.04%
==========================================
Files 48 48
Lines 2763 2770 +7
==========================================
Hits 402 402
- Misses 2361 2368 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -58,8 +58,8 @@ copernicusmarine_dataset_id(::GLORYSMonthly) = "cmems_mod_glo_phy_my_0.083deg_P1 | |||
struct CMEMSHourlyAnalysis <: CopernicusDataset end | |||
copernicusmarine_dataset_id(::CMEMSHourlyAnalysis) = "cmems_mod_glo_phy_anfc_0.083deg_PT1H-m" | |||
|
|||
CopernicusMetadata{D} = Metadata{<:CopernicusDataset, D} | |||
CopernicusMetadatum = Metadatum{<:CopernicusDataset} | |||
const CopernicusMetadata{D} = Metadata{<:CopernicusDataset, D} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metadata has 3 params, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I am not sure they are all needed because this is just dispatch type. For example
const ECCOMetadata{D} = Metadata{<:SomeECCODataset, D}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this, then I'll add the extra parameter introduced in #489 everywhere.
See the comment below: