chore: Updated for AI readiness for this repo.#38
chore: Updated for AI readiness for this repo.#38dlabaj wants to merge 2 commits intopatternfly:mainfrom
Conversation
dlabaj
requested review from
a team and
cdcabrera
and removed request for
a team
WalkthroughThis PR reorganizes test infrastructure by moving tests from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/markdownlint.yml (1)
3-11: Broaden the trigger paths for markdownlint.Only Markdown file changes will run this job, so edits to
.markdownlint.json,.markdownlintignore, or this workflow itself can change lint behavior without any CI validation. Consider adding those files to the filter.Suggested path filter
on: pull_request: paths: - - '**.md' + - '**.md' + - '.markdownlint.json' + - '.markdownlintignore' + - '.github/workflows/markdownlint.yml' push: branches: - main paths: - - '**.md' + - '**.md' + - '.markdownlint.json' + - '.markdownlintignore' + - '.github/workflows/markdownlint.yml'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/markdownlint.yml around lines 3 - 11, Update the workflow trigger paths so the markdownlint job also runs when config or workflow files that affect linting change: modify the pull_request and push paths arrays (the "pull_request: paths" and "push: paths" entries shown in the diff) to include patterns for .markdownlint.json, .markdownlintignore, and the workflow file itself (e.g., add '**/.markdownlint.json', '**/.markdownlintignore', and '.github/workflows/markdownlint.yml' or equivalent globs) so changes to those files will trigger the job.package.json (1)
21-23: Keep the relocated tests under ESLint coverage.
lint:jsstill only runs onsrc, so the newtests/**/*.test.tstree is no longer linted. If repo-wide TypeScript linting is the goal, extend this target or add a dedicated test lint script.♻️ Suggested fix
- "lint:js": "eslint src", - "lint:js:fix": "eslint src --fix", + "lint:js": "eslint src tests", + "lint:js:fix": "eslint src tests --fix",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 21 - 23, The "lint:js" npm script currently only targets "src" and therefore omits the relocated tests; update the "lint:js" script (and optionally create a new "lint:tests" script) so ESLint runs against both source and tests (e.g., include "tests/**/*.test.ts" or a broader pattern like "{src,tests}/**/*.{ts,tsx,js,jsx}") by changing the "lint:js" script (and adding "lint:tests" if you prefer separate linting) so test files are covered by ESLint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/pull_request_template.md:
- Around line 36-39: Replace the incomplete checklist entry "- [ ] Manually
tested locally (`npm run build && npm install -g`)" with a concrete or neutral
placeholder global install command; for example change it to "- [ ] Manually
tested locally (`npm run build && npm install -g <package-name>`)” or "- [ ]
Manually tested locally (`npm run build && npm install -g .`)" so the
instruction is copy-pasteable and unambiguous.
In `@CLAUDE.md`:
- Around line 42-46: The local install example is incorrect: replace the
incomplete `npm install -g` instruction with a working command such as `npm
install -g .` or instruct to run `npm link` so the package is actually installed
globally; update the block showing the three commands (the lines containing `npm
run build` and the global install) to use one of these correct install options
so `'patternfly-cli'`/`'pf'` runs the local build.
In `@tests/cli.test.ts`:
- Line 6: The current fixturesDir uses process.cwd(), coupling tests to the
working directory; change it to resolve relative to the test file using
__dirname (e.g. const fixturesDir = path.join(__dirname, 'fixtures') or
path.resolve(__dirname, 'fixtures')) so fixtures load reliably in CI/editors,
and make the same change in the other occurrence in tests/create.test.ts (update
the fixturesDir variable there as well).
---
Nitpick comments:
In @.github/workflows/markdownlint.yml:
- Around line 3-11: Update the workflow trigger paths so the markdownlint job
also runs when config or workflow files that affect linting change: modify the
pull_request and push paths arrays (the "pull_request: paths" and "push: paths"
entries shown in the diff) to include patterns for .markdownlint.json,
.markdownlintignore, and the workflow file itself (e.g., add
'**/.markdownlint.json', '**/.markdownlintignore', and
'.github/workflows/markdownlint.yml' or equivalent globs) so changes to those
files will trigger the job.
In `@package.json`:
- Around line 21-23: The "lint:js" npm script currently only targets "src" and
therefore omits the relocated tests; update the "lint:js" script (and optionally
create a new "lint:tests" script) so ESLint runs against both source and tests
(e.g., include "tests/**/*.test.ts" or a broader pattern like
"{src,tests}/**/*.{ts,tsx,js,jsx}") by changing the "lint:js" script (and adding
"lint:tests" if you prefer separate linting) so test files are covered by
ESLint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4bf83ff-4a6d-4263-9bd8-954d27856f73
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_Storepackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.md.github/LINTING.md.github/pull_request_template.md.github/workflows/actionlint.yml.github/workflows/markdownlint.yml.gitignore.markdownlint.json.markdownlintignoreCLAUDE.mdREADME.mdpackage.jsontests/cli.test.tstests/create.test.tstests/fixtures/invalid-template-bad-options.jsontests/fixtures/invalid-template-missing-name.jsontests/fixtures/not-array.jsontests/fixtures/valid-templates.jsontests/gh-pages.test.tstests/git-user-config.test.tstests/github.test.tstests/load.test.tstests/read-package-version.test.tstests/save.test.tstsconfig.json
| - [ ] Existing tests pass (`npm test`) | ||
| - [ ] Added new tests for changes | ||
| - [ ] Manually tested locally (`npm run build && npm install -g`) | ||
| - [ ] Linting passes (`npm run lint`) |
There was a problem hiding this comment.
Complete the example install command.
npm install -g is incomplete as written and can be copied into a PR without working. If you want to show a global install check, use a concrete package path or replace it with a neutral placeholder.
Possible fix
-- [ ] Manually tested locally (`npm run build && npm install -g`)
+- [ ] Manually tested locally (`npm run build && npm install -g .`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [ ] Existing tests pass (`npm test`) | |
| - [ ] Added new tests for changes | |
| - [ ] Manually tested locally (`npm run build && npm install -g`) | |
| - [ ] Linting passes (`npm run lint`) | |
| - [ ] Existing tests pass (`npm test`) | |
| - [ ] Added new tests for changes | |
| - [ ] Manually tested locally (`npm run build && npm install -g .`) | |
| - [ ] Linting passes (`npm run lint`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/pull_request_template.md around lines 36 - 39, Replace the
incomplete checklist entry "- [ ] Manually tested locally (`npm run build && npm
install -g`)" with a concrete or neutral placeholder global install command; for
example change it to "- [ ] Manually tested locally (`npm run build && npm
install -g <package-name>`)” or "- [ ] Manually tested locally (`npm run build
&& npm install -g .`)" so the instruction is copy-pasteable and unambiguous.
| ```bash | ||
| npm run build | ||
| npm install -g | ||
| # Now 'patternfly-cli' or 'pf' runs your local build | ||
| ``` |
There was a problem hiding this comment.
Fix the local install example.
npm install -g is incomplete on its own and does not install this package. Use npm install -g . or npm link so the documented command actually works.
♻️ Suggested fix
- npm install -g
+ npm install -g .📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| npm run build | |
| npm install -g | |
| # Now 'patternfly-cli' or 'pf' runs your local build | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 42 - 46, The local install example is incorrect:
replace the incomplete `npm install -g` instruction with a working command such
as `npm install -g .` or instruct to run `npm link` so the package is actually
installed globally; update the block showing the three commands (the lines
containing `npm run build` and the global install) to use one of these correct
install options so `'patternfly-cli'`/`'pf'` runs the local build.
| import templates from '../src/templates.js'; | ||
|
|
||
| const fixturesDir = path.join(process.cwd(), 'src', '__tests__', 'fixtures'); | ||
| const fixturesDir = path.join(process.cwd(), 'tests', 'fixtures'); |
There was a problem hiding this comment.
Avoid coupling fixture lookup to process.cwd().
This makes the suite depend on where Jest was launched from. Use a path relative to the test file so the fixtures resolve reliably in CI and from editors/other entrypoints. The same repo-root assumption also shows up in tests/create.test.ts.
Suggested fix
-const fixturesDir = path.join(process.cwd(), 'tests', 'fixtures');
+const fixturesDir = path.resolve(__dirname, 'fixtures');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli.test.ts` at line 6, The current fixturesDir uses process.cwd(),
coupling tests to the working directory; change it to resolve relative to the
test file using __dirname (e.g. const fixturesDir = path.join(__dirname,
'fixtures') or path.resolve(__dirname, 'fixtures')) so fixtures load reliably in
CI/editors, and make the same change in the other occurrence in
tests/create.test.ts (update the fixturesDir variable there as well).
There was a problem hiding this comment.
Question:
The tests you moved under "tests" I generally associate with unit tests, so having them under ./src/__tests__ was acceptable from every past codebase we've worked in.
Generally under the root "tests" directory we've placed more e2e focused testing. This typically involves not necessarily mocking anything except maybe network requests.
Did you make this change because the "agent ready" tool indicated these tests needed to be moved?
I would indicate we should consider not following "agent ready" (for this facet) since the tool isn't tailored towards node.js repos and then we can actively come up with e2e tests in subsequent work.
Dropping the tests refactor from this PR could be one solution.
| # Patternfly CLI | ||
|
|
||
| Patternfly CLI is a command-line tool designed for scaffolding projects, performing code modifications, and running project-related tasks. It aims to streamline development workflows and improve productivity. | ||
| Patternfly CLI is a command-line tool designed for scaffolding projects, performing code modifications, and |
There was a problem hiding this comment.
Patternfly should be PatternFly, looks like the title should get the change too. Might need to search/replace to find other instances.
| { | ||
| "name": "@patternfly/patternfly-cli", | ||
| "version": "1.0.0-prerelease.1", | ||
| "description": "Patternfly cli for scaffolding projects, performing code mods, and running project related tasks", |
There was a problem hiding this comment.
Patternfly cli should be PatternFly CLI might also consider project related with a hyphen project-related since its done in the README too
|
|
||
| # AI analysis | ||
| .agentready | ||
|
|
There was a problem hiding this comment.
If you're having issues with the .DS_Store you could consider adding in a
# os
.DS_Store
| on: | ||
| pull_request: | ||
| paths: | ||
| - '**.md' |
There was a problem hiding this comment.
Debating on this **.md vs **/*.md? Might need to confirm if you ever decide to drop Markdown underneath a directory
| branches: | ||
| - main | ||
| paths: | ||
| - '**.md' |
|
|
||
| ## Configuration Files | ||
|
|
||
| - [.eslintrc](../.eslintrc) - ESLint configuration (if exists) |
There was a problem hiding this comment.
So a couple of things.
-
wrong eslint config...
[eslint.config.js](../eslint.config.js)and well, it exists =) -
I've never seen a
LINTING.mdunderneath a.githubdirectory in the wild, not saying it isn't a thing, but if "agentready" is recommending this I'd say maybe we should consider a rethink since it's non-standard something. Plus a similar path could be achieved with an old skoolCONTRIBUTING.mdmaybe even something like aDEVELOPMENT.md. I also couldn't findLINTING.mdin any documentation searches ... its unique.
There was a problem hiding this comment.
I would also add in that if we're just trying to bump a score on the tool the score comes across as meaningless we'll only ever attempt to hit a made up metric thats constantly evolving.
2 participants
Updated repo so it now has missing features and structure to support agentic code development better. Repo is now scoring above 80%.
Summary by CodeRabbit