Skip to content
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

Refactor Catalogs #921

Merged
merged 9 commits into from May 13, 2024
Merged

Refactor Catalogs #921

merged 9 commits into from May 13, 2024

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented May 8, 2024

Issue addressed

Fixes #844

Explanation

Another attempt at merging #849. See that PR for more information, but I'll detail the changes I've also made here:

  1. I've updated the README in the catalogs a bit more, including how testing can and should be performed.
  2. The registry files were taken from the github.com domain instead of raw.githubusercontent.com domain meaning that HTML files were retrieved instead of the registry files themselves causing errors in pooch.
  3. I've added some minor changes to the examples to get the docs working again.
  4. Since the process of adding/modifying the predefined catalogs is a bit more complicated I've also added a separate checklist of easy mistakes to the PR template.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

As we saw during the last time this got merged, it is really difficult to see what the docs will do once it's merged. As such, I don't think there's much we can do now, but just get the PR through review, merge it again, and then deal with any issues that come up then. I'll be available to diagnose and fix any issues once this gets merged.

@savente93 savente93 marked this pull request as ready for review May 10, 2024 10:31
@savente93 savente93 requested a review from Jaapel May 10, 2024 10:31
@Tjalling-dejong Tjalling-dejong self-requested a review May 13, 2024 09:27
Copy link
Contributor

@Tjalling-dejong Tjalling-dejong left a comment

Choose a reason for hiding this comment

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

only some typos, other than that LGTM

data/catalogs/README.rst Outdated Show resolved Hide resolved
data/catalogs/README.rst Outdated Show resolved Hide resolved
Co-authored-by: Tjalling-dejong <93266159+Tjalling-dejong@users.noreply.github.com>
Copy link
Contributor

@Tjalling-dejong Tjalling-dejong left a comment

Choose a reason for hiding this comment

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

LGTM

@Tjalling-dejong Tjalling-dejong merged commit 39cc2c5 into main May 13, 2024
10 of 12 checks passed
@Tjalling-dejong Tjalling-dejong deleted the refactor-catalogs-take-3 branch May 13, 2024 13:41
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.

improve predefined catalog implementation
2 participants