Skip to content

Conversation

@dhegedus99
Copy link
Contributor

@dhegedus99 dhegedus99 commented Jan 28, 2025

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:

  • Data registry up to date (regenerate if necessary with a comment on this PR of /regenerate)
  • Documentation added (where applicable)
  • Changelog item added to changelog/

Copy link
Contributor

@lewisjared lewisjared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lewisjared
Copy link
Contributor

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

@dhegedus99
Copy link
Contributor Author

dhegedus99 commented Jan 29, 2025

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

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?

@lewisjared
Copy link
Contributor

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

@lewisjared
Copy link
Contributor

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?

@lewisjared
Copy link
Contributor

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. uv run pre-commit install

@lewisjared
Copy link
Contributor

/regenerate

@lewisjared
Copy link
Contributor

#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

@github-actions
Copy link

github-actions bot commented Feb 4, 2025

PR comment handling
The regenerate task is done!

You can find the workflow here:
https://github.com/CMIP-REF/ref-sample-data/actions/runs/13129171340

@lewisjared lewisjared merged commit b6a66fd into Climate-REF:main Feb 4, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants