Skip to content

Conversation

olafurpg
Copy link
Contributor

@olafurpg olafurpg commented Aug 26, 2022

Test plan

I ran npx ts-node ~/dev/sourcegraph/scip-typescript/src/main.ts index --yarn-berry-workspaces locally and it successfully indexed the Sourcegraph repo from this PR here https://github.com/sourcegraph/sourcegraph/pull/39728

@olafurpg olafurpg requested a review from valerybugakov August 26, 2022 05:57
@olafurpg olafurpg marked this pull request as ready for review August 26, 2022 05:58
Copy link
Contributor Author

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

I think we can automatically detect Yarn v3 in the future so that users don't need to use different flags based on what Yarn version they have, but for now I think it's OK to implement v3 support through a separate flag.

.command('index')
.option('--cwd <path>', 'the working directory', process.cwd())
.option('--yarn-workspaces', 'whether to index all yarn workspaces', false)
.option('--yarn-berry-workspaces', 'whether to index all yarn v3 workspaces', false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valerybugakov would it make sense to name this flag --yarn-v3-workspaces?

Copy link
Member

Choose a reason for hiding this comment

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

@olafurpg, my rationale behind berry naming here is that this API was changed in the v2 and probably will be stable in v4 — it came with the berry rewrite of yarn. Open to other ideas, but tieing it to the specific yarn version can be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Thank you for your quick response, Olaf! 💜

@olafurpg olafurpg merged commit 6c09206 into main Aug 26, 2022
@olafurpg olafurpg deleted the olafurpg/berry branch August 26, 2022 06:09
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