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

CTE generation follow up #512

Closed
edublancas opened this issue May 24, 2023 · 12 comments
Closed

CTE generation follow up #512

edublancas opened this issue May 24, 2023 · 12 comments
Assignees

Comments

@edublancas
Copy link

edublancas commented May 24, 2023

a few extra things we should do once #166 is closed

  • deprecate %sqlrender and replace it with %sqlcmd snippets {name}
  • %sqlcmd snippets should display a table with name and snippet
  • remove warnings so using --with raises an exception (in a separate PR so we give more time for users to update) (ignore this, we'll bring back --with)
@neelasha23
Copy link

  • Update Product analytics tutorial

@edublancas
Copy link
Author

also update this test: #544

@bbeat2782
Copy link

bbeat2782 commented Jun 21, 2023

For this issue, do I update the tutorials (integrations and product analytic tutorial) by replacing %sqlrender with %sqlcmd snippets {name} and removing --with to avoid exceptions in a separate PR?

And could you provide clarification on what you meant by internal and external tutorials? What is the difference between the two?

Thanks.

@edublancas

@edublancas
Copy link
Author

For this issue, do I update the tutorials (integrations and product analytic tutorial) by replacing %sqlrender with %sqlcmd snippets {name} and removing --with to avoid exceptions in a separate PR?

yes.

And could you provide clarification on what you meant by internal and external tutorials? What is the difference between the two?

internal tutorials are the ones we have in our documentation. external ones are in other websites. let's focus on internal ones for now

@bbeat2782
Copy link

bbeat2782 commented Jun 21, 2023

Acceptance Criteria:

  1. Replace %sqlrender with %sqlcmd snippets {name} from the internal tutorials and ensure it displays a table in the first PR
  2. Remove FutureWarning associated with --with and remove --with from the internal tutorials in the second PR
  3. Add FutureWarning when calling %sqlrender that tells users to use %sqlcmd snippets
  • The internal tutorials are referring to tutorials under User Guide, Integrations, How-To, and Tutorials tabs

@edublancas
Copy link
Author

one more thing: show a FutureWarning when calling %sqlrender and tell people that they should use %sqlcmd snippets instead (also link to the documentation).

this will help people transition smoothly before we remove %sqlrender from the codebase

@bbeat2782
Copy link

bbeat2782 commented Jun 22, 2023

I understood what I should do for 2) from the AC and that I should add code that raises FutureWarning when a user calls %sqlrender. What I don't understand yet is the 1) from the AC.

This is what we get when we call %sqlrender,
Screen Shot 2023-06-22 at 11 15 23 AM

and from what I observed, %sqlcmd snippets only takes 3 arguments that are all related to deleting a snippet. So do I modify %sqlcmd snippets so that it's able to take name of a snippet as an argument and display a table with name and the current output of %sqlrender before replacing %sqlrender from the tutorials?

@edublancas edublancas changed the title CTE generation follow ups CTE generation follow up Jun 22, 2023
@edublancas
Copy link
Author

@bbeat2782: you're right, there's no way to replicate %sqlrender with %sqlcmd snippets. let's pause this and work on adding such feature.

opened #647

@bbeat2782
Copy link

@edublancas
I found that there is an open issue with inferring CTE depencencies (#684)

So for remove warnings so using --with raises an exception (in a separate PR so we give more time for users to update) from the first comment, do I wait until #684 is closed or can I work on it now?

@edublancas
Copy link
Author

thanks for bringing this up, most likely, we'll end up bringing back --with. so ignore that and work on the other points from this issue

@bbeat2782
Copy link

@edublancas
I believe this can be closed now.

deprecate %sqlrender and replace it with %sqlcmd snippets {name}

#681

%sqlcmd snippets should display a table with name and snippet

#716

@edublancas
Copy link
Author

right, closing!

to close this automatically, you can put closes #NUMBER in your PRs. this way the issues are automatically closed when we merge the PR

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

No branches or pull requests

3 participants