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

feat/1495 rest_client: renames JSONResponsePaginator to JSONLinkPaginator #1558

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

willi-mueller
Copy link
Collaborator

@willi-mueller willi-mueller commented Jul 8, 2024

Description

Implements: #1495

Related Issues

Copy link

netlify bot commented Jul 8, 2024

Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit 982d324
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/668fd35ef538290008a65998
😎 Deploy Preview https://deploy-preview-1558--dlt-hub-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@rudolfix rudolfix 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 but this is a breaking change so what we need is

  • JSONResponsePaginator as an alias or derive it directly from JSONLinkPaginator
  • deprecate it, I think you can deprecate classes

what are you planning to do with rest_api? will it continue to work with old dlt or you will bump the required version?

@burnash
Copy link
Collaborator

burnash commented Jul 8, 2024

@willi-mueller Looks good, I agree with @rudolfix we need to deprecate JSONResponsePaginator,

@rudolfix will something like this work?

import warnings

class JSONLinkPaginator(BaseNextUrlPaginator):
    ...


class JSONResponsePaginator(JSONLinkPaginator):
    def __init__(self):
        warnings.warn("JSONResponsePaginator is deprecated and will be removed in version 1.0.0. Use JSONLinkPaginator instead.", DeprecationWarning, stacklevel=2)
        super().__init__()

Copy link
Collaborator

@burnash burnash left a comment

Choose a reason for hiding this comment

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

Great work, let's also deprecate the old class. See the comment above.

@willi-mueller willi-mueller force-pushed the feat/1495-rename-JSONResponsePaginator branch from e4748a9 to 762dd61 Compare July 8, 2024 16:51
@willi-mueller
Copy link
Collaborator Author

willi-mueller commented Jul 8, 2024

Great work, let's also deprecate the old class. See the comment above.

Thanks! I marked the old class as deprecated and rebased onto devel.

I also bumped the dlt dependency in the verified-sources because in this repo, verified-sources, the docs, examples, and tests use the new, unmerged, and thus unreleased class name.

Copy link
Collaborator

@burnash burnash 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! Thank you @willi-mueller

@willi-mueller willi-mueller dismissed rudolfix’s stale review July 18, 2024 12:23

we implemented the suggestions.

@burnash burnash merged commit 04a7b0e into devel Jul 18, 2024
52 checks passed
@burnash burnash deleted the feat/1495-rename-JSONResponsePaginator branch July 18, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants