-
Notifications
You must be signed in to change notification settings - Fork 180
Add create-baml-app script to work with pnpm workspace #1652
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
base: canary
Are you sure you want to change the base?
Conversation
- Make licenses ISC
@ashayas is attempting to deploy a commit to the Gloo Team on Vercel. A member of the Team first needs to authorize it. |
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.
❌ Changes requested. Reviewed everything up to 630bede in 2 minutes and 28 seconds
More details
- Looked at
497
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. typescript/create-baml-app/index.js:8
- Draft comment:
Redundant import from 'fs'. Consider using a consistent import style instead of both default and destructured imports. - Reason this comment was not posted:
Marked as duplicate.
2. typescript/create-baml-app/index.js:210
- Draft comment:
Using the 'touch' command to create 'requirements.txt' is not cross-platform (e.g., Windows may not support it). Consider using fs.writeFileSync instead. - Reason this comment was not posted:
Marked as duplicate.
3. typescript/create-baml-app/index.js:308
- Draft comment:
The Deno branch calls 'deno init', but Deno may not support this command. Verify if this is valid or consider manually creating 'deno.json'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
- The comment is speculative - it says "may not support" without evidence. 2. The code already has error handling. 3. If the command fails, the error will be caught and handled gracefully. 4. The comment asks for verification rather than making a clear assertion. 5. This is the kind of thing that would be caught immediately in testing.
I could be wrong about Deno's command line interface - maybe there really is noinit
command and this would always fail.
Even if the command doesn't exist, the error handling will catch it and provide a clear error message. The comment doesn't provide evidence that the command doesn't exist, and asking for verification isn't actionable.
Delete the comment. It's speculative, asks for verification rather than making a clear assertion, and the code already handles command failures appropriately.
4. typescript/create-baml-app/package.json:16
- Draft comment:
The 'author' field is empty. Consider providing appropriate author details. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_Dj43Sa4kVgY5pqCT
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
import inquirer from 'inquirer'; | ||
import chalk from 'chalk'; | ||
import { execa } from 'execa'; | ||
import { existsSync, mkdirSync } from 'fs'; |
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.
Avoid duplicative imports from 'fs'. Consider combining the destructured import and the default import into one statement.
// Initialize requirements.txt if it doesn't exist | ||
if (!existsSync('requirements.txt')) { | ||
console.log(chalk.yellow('\nInitializing new Python project with pip...')); | ||
await runCommand('touch', ['requirements.txt'], 'Creating requirements.txt'); |
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.
Using the touch
command may not work on all platforms (e.g. Windows). Consider using a Node API (like fs.writeFileSync
) to create an empty file in a cross-platform manner.
await runCommand('touch', ['requirements.txt'], 'Creating requirements.txt'); | |
fs.writeFileSync('requirements.txt', ''); |
this is awesome, but a testing flow with a couple containers would be extremely nice for more common setups. If it's part of the onboarding flow it can't fail ever. I took a look at the code and everything looks reasonable. What do you think? Thanks for contributing! |
@aaronvg I will add a few test containers and fix the issues the linter has found and update the PR. Yep my intention is to transfer the create-baml-app to you guys for sure! |
Migrated code over from http://github.com/ashayas/create-baml-app
Usage instructions in README
npm
,pnpm
,yarn
,deno
,bun
,pip
,poetry
,uv
,ruby
supportWill add Go (can even preemptively add it if I know the instructions)
Testing suite for this is interesting: I could maybe do a bunch of docker containers that have the various package managers
I did do tests with / without package managers
Important
Adds
create-baml-app
CLI tool for setting up BAML projects with support for multiple languages and package managers.create-baml-app
CLI tool for setting up BAML projects with support for Python, TypeScript, and Ruby.pip
,poetry
,uv
,npm
,pnpm
,yarn
,deno
,bun
,bundle
.index.js
: Main script for handling user input and project setup.package.json
: Defines dependencies (chalk
,execa
,inquirer
) and scripts for the CLI tool.pnpm-workspace.yaml
: Addscreate-baml-app
to the workspace packages.README.md
: Provides installation and usage instructions for the CLI tool.This description was created by
for 630bede. It will automatically update as commits are pushed.