--persist-replace argument

[tonykploomber]

Describe your changes

Add --persist-replace argument

Issue number

Closes #154

Checklist before requesting a review

• Performed a self-review of my code
• Formatted my code with pkgmt format
• Added tests (when necessary).
• Added docstring documentation and update the changelog (when needed)

---

📚 Documentation preview 📚: https://jupysql--440.org.readthedocs.build/en/440/

[yafimvo]

added some comments.

Question - can we detect if the user tries to use --persist for the 2nd time and suggest him to use --persist-replace instead?

[tonykploomber]

@yafimvo

I checked the preview doc, somehow there is the empty cell in the original --persist section, do you think our PR affect this?

[tonykploomber]

@idomic

Now if we use --persist twice it will suggest to use --persist-replace

[yafimvo]

Added some minor comments

[idomic]

Now if we use --persist twice it will suggest to use --persist-replace

I assume this isn't an expected behavior? If not fix it, if yes document it
Also I didn't get your point on the empty cell, we shouldn't have empty cells, if it's like that in the docs we should fix it.

[tonykploomber]

Still figuring out why the empty cell was generated...

[yafimvo]

checked the preview doc, somehow there is the empty cell in the original --persist section, do you think our PR affect this?

I don't think it's related to your changes in this PR, but it might be a good idea to remove it since it's a small and simple change that will make the documentation cleaner without affecting anything else.

[idomic]

@tonykploomber please fix and resolve comments.

[yafimvo]

I think we're missing the most important part of the test. We need to make sure --persist-replace overrides the previous data. To do it, we need to use 2 different datasets.

saved_df_name = get_table_rows_as_dataframe(ip, table_name=test_table)
ip.run_cell(f"%sql --persist sqlite:// {saved_df_name}")
saved_df_name = get_table_rows_as_dataframe(ip, table_name=test_table)
ip.run_cell(f"%sql --persist-replace sqlite:// {saved_df_name}")
assert out.result == expected_result

even if --persist-replace doesn't work, it will return True.
Please note, this is valid for the contrapositive tests as well (i.e. test_persist_replace_no_override). If you are using the same data, it will be difficult to determine whether it was not overridden.

[tonykploomber]

@yafimvo
Actually we've discussed this about this is previous comment. That's why previous query_and_save_as_dataframe function has limit parameter to generate a different dataset to test the override effect.

We can also just get the different dataframe by putting different table_name to get_table_rows_as_dataframe.

Please review again

[yafimvo]

Actually we've discussed this about this is previous comment. That's why previous query_and_save_as_dataframe function has limit parameter to generate a different dataset to test the override effect.

Since we didn't check for different lengths, I suggested setting the limit parameter as optional and making this function return X rows of a given table as a dataframe.

We can also just get the different dataframe by putting different table_name to get_table_rows_as_dataframe.

I see you implemented something like this.

LGTM. just minor fixes.

[tonykploomber]

This is blocked by CI test failing issue

[tonykploomber]

@yafimvo This is ready for review again

[edublancas]

it looks good overall, added some minor comments

[edublancas]

just show a proper warning

[tonykploomber]

Please review again

[edublancas]

looks like some rebase went bad? I see some entries in the changelog are deleted

[tonykploomber]

Can you check again? I rebase again