Repository navigation
Restructure code for fetching data into classes - #12
Conversation
…r extension for different datatypes, currently only has cmip6 as a class
There was a problem hiding this comment.
Nice. I probably would have used a protocol instead of an abstract base class because they don't really share any common functionality, but either approach works and the choice between them is personal preference. The nuance there is that if an object looks like the protocol it can be used, where as an ABC forces the use of inheritance.
|
You should also either remove the old file if you are intending to change the name of the script or change the existing script so that it is easier to see the diff that you are proposing. You can also make a PR between your current branch and your branch from #11 to only include those differences. Then in the description you can state that this PR depends on #11 |
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Capitalising class Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
chore: Update lock file
Just checking if I should do this for the changes in this PR, or if this is how I should aim to create PRs for future related edits? I wasn't sure if I should keep the old version of the script when submitting the PR, which is why I created a new file with a different name. I can replace the old file with the new file (keeping the same filename) in this PR, if that is okay? |
Yes please. That would be good. Then you can see what changes have been made compared to the original |
|
You have a merge conflict because there was some change to the other branch that aren't in this. Do you want me to resolve them or do you want to have a go? |
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
Co-authored-by: Jared Lewis <jared@jared.kiwi.nz>
|
I think you are just missing a trailing newline which is causing the pre-commit test to fail. I recommend configuring pre-commit to run the same tests on each commit. That will catch these issues without any intervention. |
|
/regenerate |
|
#11 included the changelog for this PR. Once the registry is regenerated I'll merge. Thanks for the perseverance @dhegedus99 I might have lead you astray by trying to split these #11 and #12, but hopefully the process was still somewhat useful |
|
PR comment handling You can find the workflow here: |
2 participants
Description
Reorganised code from "scripts/fetch_test_data.py" into classes in a new file "scripts/sample_dataset_request.py". This includes an abstract class for DataRequest, and two different strategy classes CMIP6Request and obs4MIPsRequest. This should make it easier to fetch more sample datasets. This is a follow-up of PR #11 and related to issue #10. The decimation function hasn't been changed yet, this PR is purely about fetching the data.
Checklist
Please confirm that this pull request has done the following:
/regenerate)changelog/