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

added zip function in textbase cli tool #102

Merged
merged 8 commits into from
Sep 8, 2023

Conversation

jahanvir
Copy link
Contributor

@jahanvir jahanvir commented Sep 3, 2023

This is an extension of previous pull request

#97

Scope

create zip file of bot using textbase CLI tool

Example:
poetry run python textbase/textbase_cli.py compress
textbase-client compress

  • [Sub task]

Screenshots


image

Developer checklist

  • I’ve manually tested that code works locally on desktop and mobile browsers.
  • I’ve reviewed my code.
  • I’ve removed all my personal credentials (API keys etc.) from the code.

@vercel
Copy link

vercel bot commented Sep 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
textbase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 8, 2023 7:15am

@sammyCofactory
Copy link
Contributor

sammyCofactory commented Sep 6, 2023

Hey @jahanvir, thanks for contributing to textbase! Now, there are a few issues with your code:

  1. If the files main.py and requirements.txt don't exist, there is still an empty zip archive being made. The error message which is shown is sufficient but the execution is not being stopped there.
  2. Adding a --path flag will be really helpful as currently the users have no choice but to always put their main.py and requirements.txt inside the textbase folder. The way this flag will work is, it'll first check if those two main files are present in the pwd (i.e., the default value for the --path parameter is the current working directory):
    • If they are, then zip everything in the pwd.
    • If they are not, then tell the user to point it to a folder which does and the zip up everything in that particular folder.

…ain.py or requirements.txt is not present. Added --path flag to take the path of both the files from user.
@jahanvir
Copy link
Contributor Author

jahanvir commented Sep 6, 2023

Hi @sammyCofactory,

I got your point.
I have made changes as per feedback.

  1. Added path option for main and requirement filed.
  2. Also added the function to check if filed exist on the given path or not so that it does not create an empty deploy.zip file.
image

@sammyCofactory
Copy link
Contributor

@jahanvir, took a look at your updated code. While you have added the --path parameter, it is not quite as we want it to be. I think you could modify it so there is only one --path parameter and not --main_path and --requirements_path. The way this will work has been explained in my previous comment (I have edited it for better understanding). Doing so will also remove the requirement for the extra fileExist function.

Do ask if you have any doubts.

@jahanvir
Copy link
Contributor Author

jahanvir commented Sep 6, 2023

Hi @sammyCofactory, so in this case there would be no need to show error. User would be sharing the correct path. And both main.py and requirements.txt in one folder only.

@sammyCofactory
Copy link
Contributor

Well, we should still have an error message. What if the user shares the wrong path?

Yes, the user has to give a path to a folder where he has both the files. If he doesn't, the default fallback will be to the current folder.

@jahanvir
Copy link
Contributor Author

jahanvir commented Sep 6, 2023

Okay, @sammyCofactory

Just to confirm user will enter the path of directory not the complete path of main.py or requirements.txt right ??

so suppose the case is

folder1/folder2/main.py

User will enter folder1/folder2

and then check needs to be run for

path + main.py and path + requirements.txt

@sammyCofactory
Copy link
Contributor

Yes, exactly! Also, when the files are to be zipped up, zip all the contents of the folder path which the user gave, i.e., zip all the contents of folder2.

…d requirements.txt is in path and compress everything from directory into deploy.zip
@jahanvir
Copy link
Contributor Author

jahanvir commented Sep 7, 2023

@sammyCofactory Okay cool

I have made changes so that it will

  • take a directory path.
  • check for main.py and requirements.txt, raise error in case its not present and stop execution there
  • compress everything present in the directory into deploy.zip

Sample test case
Screenshot 2023-09-07 at 8 11 57 PM
p execution.

Sample deply.zip file
image

In case main.py is not present in directory,
image

@sammyCofactory
Copy link
Contributor

@jahanvir, there is one small thing I observed while testing out your code. There is a deploy.zip being created inside deploy.zip. Do run the code yourself and open deploy.zip if this seems too confusing. Can you add a check in the loop to avoid this?

@jahanvir
Copy link
Contributor Author

jahanvir commented Sep 7, 2023

@sammyCofactory yeah that's happening when directory path is root directory as newly created deploy.zip will be in the same directory too.

I have added the condition in loop to avoid that!!

@sammyCofactory sammyCofactory self-requested a review September 8, 2023 07:09
Copy link
Contributor

@sammyCofactory sammyCofactory left a comment

Choose a reason for hiding this comment

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

Went through your changes and tested out your code. LGTM!

@sammyCofactory sammyCofactory merged commit ba237aa into cofactoryai:main Sep 8, 2023
1 check passed
@jahanvir jahanvir deleted the package-zip branch September 8, 2023 07:21
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