Skip to content

Conversation

@abmusse
Copy link
Member

@abmusse abmusse commented Mar 30, 2020

Resolves #143

@abmusse abmusse requested a review from kadler March 30, 2020 17:34
Copy link
Member

@kadler kadler left a comment

Choose a reason for hiding this comment

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

Good first pass. Lots of good info here. In addition to the specific comments, all the "removed at a later time" should be "removed in the next major version".

@abmusse abmusse requested a review from kadler March 30, 2020 20:01
@abmusse abmusse requested a review from kadler April 6, 2020 14:26
@abmusse
Copy link
Member Author

abmusse commented Apr 6, 2020

@kadler

I have added some updates, please re-review.

@kadler
Copy link
Member

kadler commented Apr 6, 2020

Let's land this after #173 is done.

@kadler
Copy link
Member

kadler commented Apr 22, 2020

Now that we have sphinx set up, @abmusse can you focus on getting this setup within sphinx. I think we should have a migration guide section in the nav bar and hook it in there. Also would be good to link to it from the main intro, adding some text there.

@abmusse
Copy link
Member Author

abmusse commented Apr 22, 2020

@kadler

I had issues getting the emoji :rotating_light: to render properly in read the docs

image

I ended up copying UTF-8 Unicode Character 🚨 and pasting it into the migration guide.

image

Better but does not have the styling.

@kadler
Copy link
Member

kadler commented Apr 22, 2020

If we convert it to rst, we can use admonitions. Not sure if they are supported by mkdocs.

@kadler
Copy link
Member

kadler commented Apr 22, 2020

Here's what various ones look like in readthedocs:
image

@abmusse
Copy link
Member Author

abmusse commented Apr 22, 2020

Yeah converting the migration guide to rst makes sense given that the rest of the doc files are in rst already.

@abmusse
Copy link
Member Author

abmusse commented Apr 23, 2020

@kadler I've converted the migration guide from markdown to rst

@abmusse
Copy link
Member Author

abmusse commented Apr 24, 2020

@kadler This is ready for re-review

Comment on lines +106 to +107
Migrating from ``iConn.run()`` to ``Connection.run()``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

This section is way more important to highlight than the prior one. You should be able to figure out how to connect using Connection from the docs, however you may not realize that iConn -> Connection implies a change in behavior of the callbacks. I think we definitely need some WARNING messages above to point the user at this section and mention returnError.

Something like:

"Beware that the Connection and iConn classes differ in how they call the callbacks passed to their run methods. You cannot simply replace iConn with Connection without adjusting your callbacks. See here for more info on how to do that.

@abmusse abmusse requested a review from kadler April 24, 2020 17:18
@abmusse abmusse requested a review from kadler April 24, 2020 17:59
@kadler kadler merged commit 8c7bd79 into IBM:master Apr 24, 2020
@abmusse abmusse deleted the migration-guide branch May 8, 2020 02:50
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.

Create V0 -> V1 migration guide

2 participants