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

Add deprecation warnings in the docs for commands being removed in 0.19.0 #1912

Merged
merged 10 commits into from
Oct 24, 2022

Conversation

jmholzer
Copy link
Contributor

@jmholzer jmholzer commented Oct 6, 2022

Signed-off-by: Jannic Holzer jannic.holzer@quantumblack.com

Description

In Kedro version 0.19.0, six commands are being deprecated (#1617). This PR adds deprecation notices for these commands to the documentation.

Development notes

I manually checked that the docs still build successfully.

Checklist

  • Read the contributing guidelines
  • 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

@jmholzer jmholzer marked this pull request as ready for review October 6, 2022 22:07
Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB left a comment

Choose a reason for hiding this comment

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

Good catch on this!

@merelcht
Copy link
Member

merelcht commented Oct 7, 2022

We're not removing those commands until 0.19.0 so personally I'd keep the docs until we remove them fully. Instead, we could update the mentions to indicate kedro test and kedro lint are being deprecated and point people to the new docs so they can start using the new flow already.

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@jmholzer jmholzer force-pushed the docs/remove-references-to-kedro-test branch from 46ce700 to ecd8210 Compare October 7, 2022 10:45
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@jmholzer jmholzer changed the title Remove references to kedro test and kedro lint in docs Add deprecation warnings for commands being removed in 0.19.0 Oct 7, 2022
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.

Some minor comments but other than that this looks good 👍

* [`kedro activate-nbstripout`](#strip-output-cells)
* [`kedro build-docs`](#build-the-project-documentation)
* [`kedro build-reqs`](#build-the-projects-dependency-tree)
* [`kedro activate-nbstripout (deprecated from version 0.19.0)`](#strip-output-cells)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* [`kedro activate-nbstripout (deprecated from version 0.19.0)`](#strip-output-cells)
* [`kedro activate-nbstripout`](#strip-output-cells)(deprecated from version 0.19.0)

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 change the formatting so that "(deprecated from version 0.19.0)" is not part of the link. Also for all the ones below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commits.

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 mean to remove the "(deprecated from version 0.19.0)" completely, just to have it outside of the linked text so it would be: kedro activate-nbstripout(deprecated from version 0.19.0) etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I fixed this in the latest commit.

Comment on lines 282 to 284
___
_This command will be deprecated from Kedro version 0.19.0._
___
Copy link
Member

Choose a reason for hiding this comment

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

We don't really use this style of formatting anywhere else. Perhaps just add this as a {note} or with > indenting.

See https://github.com/kedro-org/kedro/blob/main/docs/source/tutorial/create_pipelines.md for example of "notes"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand, I fixed it to use note syntax in the latest commits.

@@ -390,6 +402,10 @@ The following runs all `pytest` unit tests found in `src/tests`, including cover
kedro test
```

___
Copy link
Member

Choose a reason for hiding this comment

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

For the kedro test and kedro lint commands we can link to the new doc pages.

Copy link
Contributor Author

@jmholzer jmholzer Oct 7, 2022

Choose a reason for hiding this comment

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

I agree this is a good idea, though I would prefer to wait until #1904 is merged, so that the path to the new docs page is concrete.

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Looks good, not much too added. Agree with Merel we should stick with the {note} style.

jmholzer and others added 4 commits October 7, 2022 17:20
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
…ro-org/kedro into docs/remove-references-to-kedro-test

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@jmholzer
Copy link
Contributor Author

jmholzer commented Oct 7, 2022

Thanks for the reviews @noklam and @MerelTheisenQB!

I made the suggested changes.

@jmholzer jmholzer requested review from merelcht and noklam October 7, 2022 16:34
@jmholzer jmholzer changed the title Add deprecation warnings for commands being removed in 0.19.0 Add deprecation warnings in the docs for commands being removed in 0.19.0 Oct 7, 2022
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
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! 😄

@jmholzer jmholzer merged commit f8a699f into main Oct 24, 2022
@jmholzer jmholzer deleted the docs/remove-references-to-kedro-test branch October 24, 2022 13:02
nickolasrm pushed a commit to ProjetaAi/kedro that referenced this pull request Oct 26, 2022
…19.0 (kedro-org#1912)

* Add deprecation warnings to commands_reference.md

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>

* Move deprecation notes to fix 'phase transition' at beginning of section

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>

* Change positioning of deprecation messages to be under bash line

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>

* Change deprecation message formatting

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>

* Fix note syntax

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>

* Add deprecation note to list of contents

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Co-authored-by: Nok Lam Chan <mediumnok@gmail.com>
Signed-off-by: nickolasrm <nickolasrochamachado@gmail.com>
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