feat(config): auto-detect supported config files in the working directory#901
feat(config): auto-detect supported config files in the working directory#901AysajanE wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview Adds Reviewed by Cursor Bugbot for commit 4c8e448. Bugbot is set up for automated code reviews on this repo. Configure here. |
AugustinMauroy
left a comment
There was a problem hiding this comment.
adding blocker since feature isn't correctly covered.
thanks @AysajanE for you effort here
|
Thanks for the specific direction, @AugustinMauroy and @avivkeller. I have updated and rebased the branch to cover the full supported configuration-file set. When --config-file is omitted, discovery now checks this exact priority order:
The first existing file wins. An explicit configFile still bypasses discovery, JSON is not included because it is unsupported, and the no-file behavior remains unchanged. Coverage now includes:
Local verification on current upstream main b9750dc:
I also checked the still-open dynamic-configuration PR #905. It touches the same configuration area but implements dynamic default values, so there is no semantic overlap with this file-discovery correction at present. The updated head is 662687f. Could you please take another look when CI is ready? |
|
I feel like a dep like cosmiconfig does this for us? |
|
Didn't mean to close! Feel free to iterate based on my feedback |
|
Thanks — that makes sense. I have reworked the revision locally to use cosmiconfig for discovery instead of maintaining the file-probing logic ourselves. The explorer is configured for the current working directory and the agreed six doc-kit.config.* names, in priority order; an explicit --config-file still bypasses discovery, and the existing loader remains responsible for JavaScript and TypeScript module loading. Local verification is green: focused configuration tests 17/17, full suite 527/527, six-extension plus priority smoke check passed, lint has 0 errors (2 pre-existing unrelated warnings), and formatting passed. I have not updated the PR head yet. |
|
Can you push those changes? |
Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
|
Thanks — I’ve pushed the cosmiconfig revision in Configuration discovery is now scoped to the current working directory and uses the agreed priority order for Validation after the update:
Please take another look when convenient. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #901 +/- ##
==========================================
- Coverage 86.17% 84.96% -1.21%
==========================================
Files 188 192 +4
Lines 17146 17558 +412
Branches 1540 1559 +19
==========================================
+ Hits 14776 14919 +143
- Misses 2363 2633 +270
+ Partials 7 6 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| await cosmiconfig('doc-kit', { | ||
| searchPlaces: CONFIG_FILE_NAMES, | ||
| loaders: CONFIG_LOADERS, | ||
| searchStrategy: 'none', | ||
| }).search(process.cwd()) |
There was a problem hiding this comment.
Why is this so complex?
Can we not just:
const explorer = cosmiconfig('doc-kit');
const result = await explorer.search();
There was a problem hiding this comment.
You are right that search() already starts from process.cwd() and defaults to a non-traversing search, so the explicit working directory and searchStrategy are redundant.
The additional searchPlaces and loaders were preserving the six filenames requested in the earlier review: doc-kit.config.js, .cjs, .mjs, .ts, .cts, and .mts, with a defined priority. In particular, cosmiconfig defaults do not discover .cts or .mts, and using its defaults would also broaden discovery to package/rc/JSON/YAML conventions.
I can simplify this to cosmiconfig("doc-kit").search() if that earlier six-extension requirement is no longer desired. That would intentionally drop automatic .cts/.mts discovery and follow the standard cosmiconfig conventions instead, while explicit --config-file paths would remain unchanged. Could you confirm which behavior you prefer?
There was a problem hiding this comment.
I'd prefer that you simplified this, as I had requested, regardless of MTS/TS support.
I'd also prefer to I spoke to a human :-)
There was a problem hiding this comment.
Got it. I am reading every comment here as the real person Aysajan Eziz, as I am typing this manually.
Signed-off-by: Aysajan Eziz <aysajan1986@gmail.com>
|
Thanks, @avivkeller. @AugustinMauroy, the current head ( |
There was a problem hiding this comment.
I believe this pull request was submitted in a way that conflicts with the transparency and good-faith collaboration that open source depends on.
From what I can tell, it appears that this project and our volunteer maintainers have been used as part of Northset's product demonstration efforts. We were never asked whether we wished to participate in those efforts, nor do we endorse or intend to help market or validate commercial AI products through our review process.
For that reason, I am not comfortable accepting this contribution, regardless of its technical merits (who are only merit-worthy after heavy review by myself). I can't speak for the Node.js Project, but I personally ask that our project, and other unconsenting open-source projects, not be used as part of future promotional campaigns without prior consent, it wastes maintainer time and lowers our productivity.
Accordingly, I’m closing this pull request. However, I encourage future contributions to follow the usual open-source workflow, with contributors taking clear personal ownership of their submissions rather than presenting them as part of an external agentic verification program. I look forward to your future contributions, sent in a personal capacity rather than a marketing stunt.
(Disclaimer: I'm not a member of the Node.js TSC, or the OpenJS Foundation's staff, so I am not speaking on their behalf. I'm speaking as a maintainer who feels that this is a violation of our review efforts)
Problem
doc-kit generateonly loads configuration when--config-fileis passed explicitly. As raised in #897, a conventional project-level configuration file is otherwise silently ignored.Change
--config-fileis omitted, usecosmiconfig('doc-kit').search()from the current working directory and merge the discovered configuration. An explicit--config-filestill bypasses discovery, and the no-file fallback remains unchanged.cosmiconfigas a runtime dependency, add a patch changeset, and cover discovery, explicit-path precedence, and the no-file fallback with unit tests.Checks (Node 22)
Fixes #897
Disclosure: this change was prepared with AI assistance and reviewed by me before submitting. The linked Northset receipt records the original submitted revision; the current head includes later maintainer-requested revisions and is not covered by that original immutable run record. I self-ran the checks listed above on the current revision. Contributor self-run, not maintainer verification.
https://northset-oss.github.io/verification-pilot/