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

docs: Reorganize docs and add deprecation timeline #1353

Merged
merged 9 commits into from
Feb 8, 2023
Merged

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Jan 26, 2023

Extracted from #1294


📚 Documentation preview 📚: https://meltano-sdk--1353.org.readthedocs.build/en/1353/

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #1353 (8985332) into main (89adc1b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1353   +/-   ##
=======================================
  Coverage   85.19%   85.19%           
=======================================
  Files          54       54           
  Lines        4722     4722           
  Branches      803      803           
=======================================
  Hits         4023     4023           
  Misses        507      507           
  Partials      192      192           
Impacted Files Coverage Δ
singer_sdk/typing.py 94.94% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tayloramurphy
Copy link
Collaborator

@edgarrmondragon is it possible to specify redirects? I know we've linked a bunch of folks directly to the current batch and inline stream maps links on the sidebar.

@edgarrmondragon
Copy link
Collaborator Author

@edgarrmondragon is it possible to specify redirects? I know we've linked a bunch of folks directly to the current batch and inline stream maps links on the sidebar.

Yeah, let me try adding the redirects.

@edgarrmondragon
Copy link
Collaborator Author

edgarrmondragon commented Jan 27, 2023

@tayloramurphy
Copy link
Collaborator

@edgarrmondragon Awesome!

I think it'd be worth getting Sven to review this PR as well.

@tayloramurphy tayloramurphy requested review from sbalnojan and removed request for afolson January 30, 2023 18:31
@aaronsteers
Copy link
Contributor

aaronsteers commented Feb 2, 2023

Merged in the contributions from @kgpayne in #1381.

Here are my copy-pasted feedback items from that PR. The only ones I feel really strongly about are the ones that will change the hosted URL, since those will be another breaking change if held for later.


Just focusing on nav structure, for which we want to reduce later breaking changes:

  1. I think the three-level makes this a bit tricky to find. Since we already have another "Guide" at top level, how about moving these up a level to be either .../migration/<pagename> ("Migration Guides" as the parent title) or ../guides/<pagename> ("In Depth Guides" as the parent title):
  • image

  1. I realize "Advanced" may now be redundant with the header for that section. Should we remove the additional .../advanced/... directory structure, and just let them float into the "Advanced" Section header?
    • image

The other nesting looks great! I see .../implementation/..., .../classes/... - those make sense and seem stable to me.


Some other small reorg notes, none of which are breaking so if we want to hold for now, we totally can:

Some titles have redundant references which we can clean up. None of these naming issues need to be part of this PR, per se, but if it's easy enough to clean them up, then great. 👍

  1. Python Tips for SDK Developers can simply be "Python Tips".
  2. SDK Code Samples could be "Code Samples".
  3. SDK Dev Guide can be "Dev Guide" or "Getting Started"
  4. SDK Reference could be "Classes Reference" or "SDK Classes"
  5. SDK Implementation Details could be "Singer Implementation Details".
    • Most/all of those topics are really about how the SDK (internally) handles one Singer-specific detail or another.
  6. SDK Implementation Details - Catalog Discovery can be "Catalog Discovery".
  7. All the siblings of "Catalog Discovery" noted above can have the prefix "SDK Implementation Details - " removed. (I think those prefixes was originally created to aid in navigation when we were hosting from GitLab repo directly, without ReadTheDocs.

@edgarrmondragon - What do you think of at least including the revisions above that affect the URLs, and optionally some of the other more cosmetic items?

cc @sbalnojan as fyi

@aaronsteers
Copy link
Contributor

cc @sbalnojan

@edgarrmondragon
Copy link
Collaborator Author

@edgarrmondragon - What do you think of at least including the revisions above that affect the URLs, and optionally some of the other more cosmetic items?

@aaronsteers Yeah, that makes sense. I'll start with the URL changes.

@kgpayne
Copy link
Contributor

kgpayne commented Feb 6, 2023

@edgarrmondragon just checking if this is 'ready for review'? before I check it out in more detail 🙂

Copy link
Collaborator

@tayloramurphy tayloramurphy left a comment

Choose a reason for hiding this comment

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

Approving as I think this is a good iteration. We've got redirects and the overall content is better structured. @sbalnojan it would be good if you can do a review on this please!

@edgarrmondragon
Copy link
Collaborator Author

@edgarrmondragon just checking if this is 'ready for review'? before I check it out in more detail 🙂

@kgpayne This is ready now 😄

Copy link
Contributor

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

Thanks, @edgarrmondragon, for incorporating the latest changes. Looks great!

@kgpayne kgpayne merged commit 92dd45a into main Feb 8, 2023
@kgpayne kgpayne deleted the docs/reorganize branch February 8, 2023 10:00
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