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

Remove mentions of ConfigLoader and TemplatedConfigLoader from docs #3257

Merged
merged 8 commits into from
Nov 2, 2023

Conversation

ankatiyar
Copy link
Contributor

@ankatiyar ankatiyar commented Nov 1, 2023

Description

Part of #2692

Development notes

Updated docs with the removal of ConfigLoader and TemplatedConfigLoader

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

ankatiyar and others added 6 commits November 1, 2023 11:18
Signed-off-by: Jo Stichbury <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
@ankatiyar ankatiyar marked this pull request as ready for review November 1, 2023 13:12
@ankatiyar ankatiyar requested review from noklam and merelcht and removed request for yetudada November 1, 2023 13:12
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

LGTM, deferring final approval to @stichbury

@@ -1,137 +1,48 @@
# Advanced configuration
The documentation on [configuration](./configuration_basics.md) describes how to satisfy most common requirements of standard Kedro project configuration:

By default, Kedro is set up to use the [ConfigLoader](/kedro.config.ConfigLoader) class. Kedro also provides two additional configuration loaders with more advanced functionality: the [TemplatedConfigLoader](/kedro.config.TemplatedConfigLoader) and the [OmegaConfigLoader](/kedro.config.OmegaConfigLoader).
Each of these classes are alternatives for the default `ConfigLoader` and have different features. The following sections describe each of these classes and their specific functionality in more detail.
By default, Kedro is set up to use the [OmegaConfigLoader](/kedro.config.OmegaConfigLoader) class.
Copy link
Member

Choose a reason for hiding this comment

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

Pet peeve but any chance we can start using cross references more often?

Suggested change
By default, Kedro is set up to use the [OmegaConfigLoader](/kedro.config.OmegaConfigLoader) class.
By default, Kedro is set up to use the {py:class}`~kedro.config.OmegaConfigLoader` class.

https://myst-parser.readthedocs.io/en/latest/syntax/cross-referencing.html#reference-roles

Copy link
Contributor

Choose a reason for hiding this comment

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

Pet peeve but any chance we can start using cross references more often?

That'll all come when we do the linking work in #3015 (we can fix each and every link). But I'm dreading it, what a horrid syntax 🤮

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, should we update to use the myst syntax for page links [my other page](../../page.md) and anchors to subheads too, or just to API docs? @astrojuanlu

Copy link
Member

Choose a reason for hiding this comment

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

I didn't want to suggest a sweeping change to page xrefs too (because I'm mindful the syntax is a bit weird), maybe we can start with API objects and see if we get used to it and start using it more?

Copy link
Member

Choose a reason for hiding this comment

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

Also this syntax does not depend on intersphinx, mind you. It's the same whether the object being linked is on this project or another one @stichbury

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stichbury @astrojuanlu Do I need to make these changes now or will this be a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favour of replacing references to OmegaConfigLoader with this, but I defer the final call to @stichbury

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do it as a separate PR so @ankatiyar can get this one out and we can make a set of changes wholesale later.

docs/source/configuration/config_loader_migration.md Outdated Show resolved Hide resolved
Co-authored-by: Jo Stichbury <[email protected]>
Signed-off-by: Ankita Katiyar <[email protected]>
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking on this heap of changes @ankatiyar 🏅 🏆

@ankatiyar ankatiyar enabled auto-merge (squash) November 2, 2023 15:30
@ankatiyar ankatiyar merged commit 39e265a into develop Nov 2, 2023
23 of 33 checks passed
@ankatiyar ankatiyar deleted the docs/configuration branch November 2, 2023 15:40
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.

4 participants