-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(examples): replace pnpm in readme #6747
base: main
Are you sure you want to change the base?
feat(examples): replace pnpm in readme #6747
Conversation
… per selectedPackageManager
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
@DependerKumarSoni is attempting to deploy a commit to the Vercel 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.
Nice little QoL improvement, thanks!
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.
Thanks for taking this on! Left a few comments on the approach, let me know if you have more questions and I'm happy to help.
packages/create-turbo/src/commands/create/updateCommandsInREADME.js
Outdated
Show resolved
Hide resolved
packages/create-turbo/src/commands/create/updateCommandsInREADME.js
Outdated
Show resolved
Hide resolved
packages/create-turbo/src/commands/create/updateCommandsInREADME.js
Outdated
Show resolved
Hide resolved
packages/create-turbo/src/commands/create/updateCommandsInREADME.js
Outdated
Show resolved
Hide resolved
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.
Due to this age of this PR, I'd like to merge it without tests. I'd rather unblock this PR and add tests in a separate PR if we do believe it's needed.
Thanks, @DependerKumarSoni and sorry for this taking so long to get merged!
Thank you, @anthonyshew, for reviewing and considering this PR. I understand the concern about its age and truly appreciate your efforts to unblock it. I noticed the message: "Merging is blocked: Merging can be performed automatically once the requested changes are addressed." While I believe I’ve addressed the requested changes (except for the tests, which can be added later if needed), could you please clarify if there’s anything else pending on my end to move this forward? I’m happy to make any further updates required to facilitate the merge. Thank you again for your support! |
Hi @tknickman and @anthonyshew |
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.
Looks great - let's add some tests but otherwise looks great!
return { result: "success", ...meta }; | ||
} | ||
|
||
function replacePackageManager( |
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.
Let's add some test cases for this function just to make sure it's working as we expect. You can use it.each to make this simple. Something like:
const testCases = [...]
it.each(testCases)(
'should replace placeholders correctly for packageManager: %p and text: "%s"',
(packageManager, text, expected) => {
const result = replacePackageManager(packageManager, text);
expect(result).toBe(expected);
}
);
@@ -245,7 +245,7 @@ describe("create-turbo", () => { | |||
telemetry, | |||
}); | |||
|
|||
expect(mockConsole.error).toHaveBeenCalledTimes(2); | |||
expect(mockConsole.error).toHaveBeenCalledTimes(3); |
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.
Why is this error increasing?
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.
It has to do with https://github.com/vercel/turborepo/pull/6747/commits/870f012b5b67f96fa11aec0a8aafbfeaad287d3b#diff-87964d57d03186688cb5[…]bf2993f85483c259c7R31-R33. It made complete sense to me at the time that I wrote it - but now I'm struggling to remember why. 🤔
Description
This pull request introduces a JavaScript script that dynamically updates the README.md file based on the selected package manager. This enhancement aims to improve clarity and deliver additional context for users.