Improve ergonomics of setModuleDefaults/Load.scan()#903
Improve ergonomics of setModuleDefaults/Load.scan()#903jdmarshall wants to merge 1 commit intonode-config:masterfrom
Conversation
📝 WalkthroughWalkthroughThe Load.scan(additional) method now returns the resolved configuration object ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/0-util.js (1)
1715-1721: Consider usingassert.strictEqualfor consistency.The test correctly validates the new return behavior. However, the rest of this file uses
assert.strictEqualfor comparisons. For consistency, consider usingassert.strictEqualhere as well.Suggested fix
it('returns the resolved data', function() { let load = new Load({configDir: import.meta.dirname + '/config', skipConfigSources: true}); let actual = load.scan(); - assert.equal(actual, load.config); + assert.strictEqual(actual, load.config); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/0-util.js` around lines 1715 - 1721, Replace the non-strict assertion in the test that verifies Load.scan() by changing the call to assert.equal(actual, load.config) to use assert.strictEqual(actual, load.config) so it matches the rest of the file’s assertions; locate the test that constructs new Load({configDir: import.meta.dirname + '/config', skipConfigSources: true}), calls load.scan(), and assigns to actual, then update the assertion to strictEqual (no other changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/0-util.js`:
- Around line 1715-1721: Replace the non-strict assertion in the test that
verifies Load.scan() by changing the call to assert.equal(actual, load.config)
to use assert.strictEqual(actual, load.config) so it matches the rest of the
file’s assertions; locate the test that constructs new Load({configDir:
import.meta.dirname + '/config', skipConfigSources: true}), calls load.scan(),
and assigns to actual, then update the assertion to strictEqual (no other
changes required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63ae5fba-d468-4e90-b6c2-1c814ea4ede7
📒 Files selected for processing (2)
lib/util.jstest/0-util.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/util.js (1)
1098-1100: Minor: JSDoc@seereference may be incorrect.The
@seetag referencesConfigUtils.setModuleDefaults(), but the utility class in this file is namedUtil, and the method exists onLoad.setModuleDefaults()at line 1243. Consider updating to reference the correct class/method, such as@see Load.setModuleDefaults()or@see config.util.setModuleDefaults().Suggested fix
* scan and load config files in the same manner that config.js does * - * `@see` ConfigUtils.setModuleDefaults() + * `@see` Load.setModuleDefaults() * `@param` {{name: string, config: any}[]=} additional additional values to populate (usually from NODE_CONFIG) * `@returns` {object} - config data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/util.js` around lines 1098 - 1100, JSDoc `@see` references the wrong symbol; update the tag in the comment above the function to point to the actual utility method name—change `@see ConfigUtils.setModuleDefaults()` to the correct reference `@see Load.setModuleDefaults()` (or `@see config.util.setModuleDefaults()` if you prefer the external API name) so it matches the real method defined as Load.setModuleDefaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/util.js`:
- Around line 1098-1100: JSDoc `@see` references the wrong symbol; update the tag
in the comment above the function to point to the actual utility method
name—change `@see ConfigUtils.setModuleDefaults()` to the correct reference
`@see Load.setModuleDefaults()` (or `@see config.util.setModuleDefaults()` if
you prefer the external API name) so it matches the real method defined as
Load.setModuleDefaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a959445-622e-496f-9007-362eebe5bbc9
📒 Files selected for processing (2)
lib/util.jstest/0-util.js
1 participant
Fixes #901
This should simplify the setup code for moduleDefaults, which I'll update in the docs.
Summary by CodeRabbit
New Features
Tests