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

Issue#917: Add a check to know if database is archived #920

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

ashishgangaramani
Copy link
Contributor

@ashishgangaramani ashishgangaramani commented Oct 20, 2024

#917
Add a check to see if the database is archived. If yes, a warning message prompting the user to unarchive the group this database belongs to will appear.

@ashishgangaramani
Copy link
Contributor Author

I have addressed issue #917 with this pull request. If you have any feedback or need further changes, please let me know. Otherwise, if everything looks good, could you please proceed with merging it?

Thank you for your time and assistance!

Best regards,
Ashish Babish Gangaramani

@avinassh
Copy link
Member

avinassh commented Nov 1, 2024

Hi, the code doesn't exit after printing the error. It should not proceed further.

Also * within the message don't do anything when printed. Just remove them so that it prints:

Your DB might be archived. Please run `turso group unarchive <group name here>` to unarchive it
 

@ashishgangaramani
Copy link
Contributor Author

Hi,
Thanks for your prompt feedback. As you requested, I have made the changes kindly take a look.

@avinassh
Copy link
Member

avinassh commented Nov 4, 2024

Hey @ashishgangaramani, this looks good. However I missed one thing in my earlier review. Why not do the check right after if !isURL(nameOrUrl) if else block (i.e. line 187). Right now the code is checking for status when an SQL query is passed to the shell or when it is running the shell directly. But since it is doing the checks at both places anyway, we can instead do it before and fail early.

@ashishgangaramani
Copy link
Contributor Author

*re-opening pull request

@ashishgangaramani
Copy link
Contributor Author

Hi @avinassh,

Thank you for your insightful suggestions. I have implemented the changes as advised. Could you please review them and let me know if any further modifications are needed?

@avinassh avinassh merged commit 436da95 into tursodatabase:main Nov 6, 2024
2 of 3 checks passed
@avinassh
Copy link
Member

avinassh commented Nov 6, 2024

Thank you @ashishgangaramani

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.

2 participants