Conversation
9a0b040 to
c6117b9
Compare
b96d217 to
3145b8e
Compare
b39e84d to
b64ea5e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements breaking changes to how pnpm handles configuration settings, moving workspace-specific settings from .npmrc files to pnpm-workspace.yaml files, and standardizing the naming conventions between the two formats. The changes focus on separating concerns between npm-compatible settings (in .npmrc) and pnpm-specific workspace settings (in pnpm-workspace.yaml).
Key changes:
- Configuration settings now strictly enforce that workspace-specific settings must be in camelCase in
pnpm-workspace.yaml, while rc options remain in kebab-case - Auth-related information is now censored in
pnpm config listandpnpm config getcommands - Array values returned by
pnpm config getare now properly formatted as JSON arrays instead of comma-separated strings
Reviewed Changes
Copilot reviewed 44 out of 46 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/naming-cases/* | New package providing utilities to validate and check naming case conventions (camelCase, kebab-case) |
| config/plugin-commands-config/src/* | Updated config command handlers to enforce naming conventions, censor protected settings, and properly format output |
| config/config/src/* | Modified config loading to filter non-auth settings from rc files and properly handle workspace manifest settings |
| pnpm/test/* | Updated tests to use pnpm-workspace.yaml instead of .npmrc for workspace-specific settings |
| config/config/test/* | Added comprehensive tests for the new configuration loading behavior |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
config/plugin-commands-config/src/configSet.ts:1
- The test case was changed from
foo=bar=qartolockfile-dir=foo=bar, but this changes the nature of the test. The original test appeared to verify handling of arbitrary keys with multiple=signs, while the new test uses a known config key. Consider adding a separate test case for the original scenario to maintain coverage of edge cases.
import fs from 'fs'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NOTE: this block of code in this location is pointless. | ||
| // TODO: move this block of code to after the code that loads pnpm-workspace.yaml. | ||
| // TODO: unskip test `getConfig() sets mergeGiBranchLockfiles when branch matches mergeGitBranchLockfilesBranchPattern`. |
There was a problem hiding this comment.
The TODO comment identifies a known issue but the code is left in its current (incorrect) location. This creates technical debt and causes at least one test to be skipped. The code should be moved to the correct location as part of this PR, or a separate issue should be created to track this work.
|
Let's aim at merging the PR this week. If something is not done yet, it can be done in a new PR. Just have the existing tests pass. |
be9aed0 to
5b34702
Compare
4b9964c to
8b2921b
Compare
|
Overall seems fine to me. I might review it again after merge. |
| `pnpm config list` and `pnpm config get` (without argument) now show top-level keys as camelCase. | ||
| Exception: Keys that start with `@` or `//` would be preserved (their cases don't change). |
There was a problem hiding this comment.
@zkochan I just discovered that this particular changeset is a misinformation. This is a change I planned to implement though, so it will be included in a different PR.
TODO:
pnpm config list.packages) from rc files.rawConfig.pnpm config get <array>output JSON.rawConfigas camelCase instead.pnpm config get.pnpm config set.pnpm config setrefuse kebab-case workspace-specific settings.pnpm config list.pnpm config get.pnpm config set.pnpm-workspace.yaml.isCamelCase.test.ts.RemoveisStrictlyKebabCase.RenamecheckCases.tstoisCamelCase.ts.pnpm config set.pnpm/config/config/src/index.ts
Lines 290 to 292 in 22f198a
npm_config_*withpnpm_config_*for pnpm specific settings: https://gist.github.com/KSXGitHub/f4c4bf4294b32f19fcd0c0ab9bb250ccExcluded because they are not breaking changes:
--format json|ini.--iniis an alias for--format ini.--jsonis an alias for--format json.Cancelled TODO:
pnpm config listpnpm config list --jsonshould print all top-level keys in camelCase.pnpm config list --jsonshould printregistriesinstead ofregistryand@scope:registry.pnpm config list [--ini]should print workspace-specific keys in camelCase and other in kebab-case.pnpm config list --jsonshould print almost all as camelCase except@scope:registryand//domain:auth.pnpm config listshould print rc keys as kebab-case and workspace-specific keys as camelCase.pnpm get [--json] ''.