-
-
Notifications
You must be signed in to change notification settings - Fork 230
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: add ability in robyn cli to scaffold example programs with various DBs #584
Conversation
👷 Deploy request for robyn pending review.Visit the deploys page to approve it
|
CodSpeed Performance ReportMerging #584 will not alter performanceComparing Summary
|
Hey @bwdq 👋 Great work 😄 I will have a thorough look over the weekend but this great work so far! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bwdq 👋
Great work 😄 I have a few suggestions. Do let me know if you have any questions from me 😄
Also, can you please bring the Dockerfile in the scaffold folder instead of generating it on the fly?
robyn/scaffold/mongo.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these databases would also require certain dependencies. Can we update the docker files accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the Docker files for the databases. When I ran docker it errored 'uvloop not found' so uvloop should be a mandatory dependency instead of optional.
robyn/__main__.py
Outdated
project_dir = result[0] | ||
docker = result[1] | ||
project_type = result[2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project_dir = result[0] | |
docker = result[1] | |
project_type = result[2] | |
project_dir, docker, project_type = result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this and it did not parse correctly so I left it as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwdq , what do you mean by this? What error were you getting?
We can return a dictionary instead of a tuple if there is an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advise returning result as dict instead of tuple, will make it more clear what is what and avoid future issues in case a new question is added or the order changes.
robyn/scaffold/prisma/app.py
Outdated
|
||
# install prisma cli | ||
# initialize prisma with "prisma generate" | ||
# then "prisma migrate dev" and name migration dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwdq 👋
Can you please explain the line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are instructions to the user because prisma won't "just work" and requires some setup. I'm not sure what the optimal solution here is. I don't want to execute bash commands silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @bwdq 👋
We can add a small readme inside every folder. Doesn't need to be too fancy. Can just contain these commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved the python comments to README's.
robyn/scaffold/sqlite/app.py
Outdated
res = cur.execute("SELECT name FROM sqlite_master") | ||
th = res.fetchone() | ||
print(th) | ||
return "Hello World!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that we should be including res
in the final response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
table_name = th[0] return f"Hello World! {table_name}"
robyn/__main__.py
Outdated
if project_type == "sqlite": | ||
f.write(open("robyn/scaffold/sqlite/app.py", "r").read()) | ||
elif project_type == "postgres": | ||
f.write(open("robyn/scaffold/postgres/app.py", "r").read()) | ||
# copy README.md | ||
readme_path = os.path.join(project_dir, "README.md") | ||
with open(readme_path, "w") as f_r: | ||
f_r.write(open("robyn/scaffold/postgres/README.md", "r").read()) | ||
elif project_type == "mongo": | ||
f.write(open("robyn/scaffold/mongo/app.py", "r").read()) | ||
elif project_type == "sqlalchemy": | ||
f.write(open("robyn/scaffold/sqlalchemy/app.py", "r").read()) | ||
elif project_type == "prisma": | ||
f.write(open("robyn/scaffold/prisma/app.py", "r").read()) | ||
# copy schema.prisma | ||
schema_path = os.path.join(project_dir, "schema.prisma") | ||
with open(schema_path, "w") as f_s: | ||
f_s.write(open("robyn/scaffold/prisma/schema.prisma", "r").read()) | ||
# copy README.md | ||
readme_path = os.path.join(project_dir, "README.md") | ||
with open(readme_path, "w") as f_r: | ||
f_r.write(open("robyn/scaffold/prisma/README.md", "r").read()) | ||
else: | ||
f.write(open("robyn/scaffold/no-db/app.py", "r").read()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a cleaner way to implement this?
can you store the file_name and file_paths in a dictionary and then try accessing it?
@IdoKendo do you have any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would simplify this whole block to something like these 3 lines:
base_path = f"robyn/scaffold/{project_type}"
shutil.copyfile(f"{base_path}/README.md", readme_path)
shutil.copyfile(f"{base_path}/app.py", app_file_path)
Just make sure the project_type
default is no-db
😃
Can add some guard clauses to be extra safe, but I think InquirerPy
does that for you out-of-the-box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further review of the code, can even do it as simple as:
from distutils.dir_util import copy_tree
copy_tree(project_dir, f"robyn/scaffold/{project_type}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much simpler but at the time I didn't want to add a new dependency. I've switched to this now.
robyn/__main__.py
Outdated
if project_type == "sqlite": | ||
f.write(open("robyn/scaffold/sqlite/Dockerfile", "r").read()) | ||
elif project_type == "postgres": | ||
f.write(open("robyn/scaffold/postgres/Dockerfile", "r").read()) | ||
elif project_type == "mongo": | ||
f.write(open("robyn/scaffold/mongo/Dockerfile", "r").read()) | ||
elif project_type == "sqlalchemy": | ||
f.write(open("robyn/scaffold/sqlalchemy/Dockerfile", "r").read()) | ||
elif project_type == "prisma": | ||
f.write(open("robyn/scaffold/prisma/Dockerfile", "r").read()) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bwdq 👋 Nice job!
I've added some comments to simplify and improve upon the changes. LMK if there's anything I can help with!
robyn/__main__.py
Outdated
project_dir = result[0] | ||
docker = result[1] | ||
project_type = result[2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advise returning result as dict instead of tuple, will make it more clear what is what and avoid future issues in case a new question is added or the order changes.
robyn/__main__.py
Outdated
if project_type == "sqlite": | ||
f.write(open("robyn/scaffold/sqlite/app.py", "r").read()) | ||
elif project_type == "postgres": | ||
f.write(open("robyn/scaffold/postgres/app.py", "r").read()) | ||
# copy README.md | ||
readme_path = os.path.join(project_dir, "README.md") | ||
with open(readme_path, "w") as f_r: | ||
f_r.write(open("robyn/scaffold/postgres/README.md", "r").read()) | ||
elif project_type == "mongo": | ||
f.write(open("robyn/scaffold/mongo/app.py", "r").read()) | ||
elif project_type == "sqlalchemy": | ||
f.write(open("robyn/scaffold/sqlalchemy/app.py", "r").read()) | ||
elif project_type == "prisma": | ||
f.write(open("robyn/scaffold/prisma/app.py", "r").read()) | ||
# copy schema.prisma | ||
schema_path = os.path.join(project_dir, "schema.prisma") | ||
with open(schema_path, "w") as f_s: | ||
f_s.write(open("robyn/scaffold/prisma/schema.prisma", "r").read()) | ||
# copy README.md | ||
readme_path = os.path.join(project_dir, "README.md") | ||
with open(readme_path, "w") as f_r: | ||
f_r.write(open("robyn/scaffold/prisma/README.md", "r").read()) | ||
else: | ||
f.write(open("robyn/scaffold/no-db/app.py", "r").read()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further review of the code, can even do it as simple as:
from distutils.dir_util import copy_tree
copy_tree(project_dir, f"robyn/scaffold/{project_type}")
Thanks for the review @IdoKendo 😄 I wasn't aware about this feature
|
Any updates on this @bwdq ? |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I have updated and simplified the logic in main. I'm still having issues with the robyn resetting the connection when running on docker and some testing would be helpful. |
Hey @bwdq 👋
Can you please explain what you mean here? I don't understand what you mean by "resetting" the connection. |
The issue isn't really related to this PR because it's reproducible on the main branch so I've opened a separate issue for it. |
Hey @bwdq 👋 Is the PR up for review? |
Yes it is. The docker issue is now fixed. |
Hey @bwdq 👋 I have two suggestions for this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bwdq 👋
I was going trying your branch and when I run the command python -m robyn --create
I get the following error
Please select project type (Mongo/Postgres/Sqlalchemy/Prisma): Sqlite
Creating a new Robyn project '.'...
Traceback (most recent call last):
File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "/Users/sanskar/.pyenv/versions/3.10.7/lib/python3.10/runpy.py", line 86, in _run_code
exec(code, run_globals)
File "/Users/sanskar/repos/robyn/robyn/__main__.py", line 68, in <module>
create_robyn_app()
File "/Users/sanskar/repos/robyn/robyn/__main__.py", line 51, in create_robyn_app
copy_tree(f"robyn/scaffold/{project_type}/", project_dir)
File "/Users/sanskar/repos/robyn/venv/lib/python3.10/site-packages/setuptools/_distutils/dir_util.py", line 135, in copy_tree
raise DistutilsFileError("cannot copy tree '%s': not a directory" % src)
distutils.errors.DistutilsFileError: cannot copy tree 'robyn/scaffold/sqlite/': not a directory
And this is happening for every selection. Can you please have a look?
@sansyrox I'm taking about sqlalchemy + alembic migration template but we can do after we migrate create_robyn_app to another repo ( #659 ) |
@Mr-Sunglasses , can you please make an issue for the same? |
for more information, see https://pre-commit.ci
Sure. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Great work Team 🙌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
Thank you @IdoKendo and @Mr-Sunglasses 😄 ✨ |
Fixes #511 and adds some example code for scaffolding Sqlite, Postgres, Mongo, Prisma, and SQLAlchemy.