docs: Add Skill, centralize contributing docs and de-duplicate readme#572
docs: Add Skill, centralize contributing docs and de-duplicate readme#5721012Charan wants to merge 1 commit intomeshery:masterfrom
Conversation
Summary of ChangesHello @1012Charan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the project's documentation by centralizing contribution guidelines and explicitly emphasizing the Schema-Driven Development (SDD) paradigm. The changes aim to provide a clearer, more organized, and comprehensive guide for developers, ensuring that the project's core development philosophy is immediately apparent and well-supported with detailed instructions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively centralizes the contributing documentation into CONTRIBUTING.md, making the README.md more concise and focused. The emphasis on Schema-Driven Development is now clearer across the documentation. The overall structure is much improved. I've identified a few opportunities to enhance the new documentation by simplifying command examples and correcting code snippets to ensure they are complete and error-free. These suggestions aim to further improve clarity and prevent potential issues for contributors.
| ```javascript | ||
| import { combineReducers, configureStore } from "@reduxjs/toolkit"; | ||
| import { cloudApi as cloudBaseApi } from "@meshery/schemas/dist/cloudApi"; | ||
|
|
||
| const rootReducer = combineReducers({ | ||
| // your reducers... | ||
| [cloudBaseApi.reducerPath]: cloudBaseApi.reducer, | ||
| }); | ||
|
|
||
| export const store = configureStore({ | ||
| reducer: rootReducer, | ||
| middleware: getDefaultMiddleware => | ||
| getDefaultMiddleware().concat(cloudBaseApi.middleware) | ||
| }); | ||
|
|
||
| setupListeners(store.dispatch); | ||
| ``` |
There was a problem hiding this comment.
The setupListeners function is used in this Redux store configuration example, but it is not imported. This will cause an error for anyone copying this code. You should add the import from @reduxjs/toolkit/query to make the snippet complete and runnable.
| ```javascript | |
| import { combineReducers, configureStore } from "@reduxjs/toolkit"; | |
| import { cloudApi as cloudBaseApi } from "@meshery/schemas/dist/cloudApi"; | |
| const rootReducer = combineReducers({ | |
| // your reducers... | |
| [cloudBaseApi.reducerPath]: cloudBaseApi.reducer, | |
| }); | |
| export const store = configureStore({ | |
| reducer: rootReducer, | |
| middleware: getDefaultMiddleware => | |
| getDefaultMiddleware().concat(cloudBaseApi.middleware) | |
| }); | |
| setupListeners(store.dispatch); | |
| ``` | |
| import { combineReducers, configureStore } from "@reduxjs/toolkit"; | |
| import { setupListeners } from "@reduxjs/toolkit/query"; | |
| import { cloudApi as cloudBaseApi } from "@meshery/schemas/dist/cloudApi"; | |
| const rootReducer = combineReducers({ | |
| // your reducers... | |
| [cloudBaseApi.reducerPath]: cloudBaseApi.reducer, | |
| }); | |
| export const store = configureStore({ | |
| reducer: rootReducer, | |
| middleware: getDefaultMiddleware => | |
| getDefaultMiddleware().concat(cloudBaseApi.middleware) | |
| }); | |
| setupListeners(store.dispatch); |
| ```javascript | ||
| import { useGetPlansQuery, useCreateDesignMutation } from "@meshery/schemas/dist/cloudApi"; | ||
|
|
||
| function MyComponent() { | ||
| const { data: plans, isLoading, error } = useGetPlansQuery(); | ||
|
|
||
| if (isLoading) return <div>Loading...</div>; | ||
| if (error) return <div>Error loading plans</div>; | ||
|
|
||
| return ( | ||
| <div> | ||
| {plans.map(plan => <div key={plan.id}>{plan.name}</div>)} | ||
| </div> | ||
| ); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
In this React component example, the plans variable will be undefined on the initial render when isLoading is true. After isLoading becomes false, if there's no error, plans.map will be called. However, plans might still be undefined before the data is fetched, which would cause a runtime error. It's safer to use optional chaining (?.) to prevent this.
| ```javascript | |
| import { useGetPlansQuery, useCreateDesignMutation } from "@meshery/schemas/dist/cloudApi"; | |
| function MyComponent() { | |
| const { data: plans, isLoading, error } = useGetPlansQuery(); | |
| if (isLoading) return <div>Loading...</div>; | |
| if (error) return <div>Error loading plans</div>; | |
| return ( | |
| <div> | |
| {plans.map(plan => <div key={plan.id}>{plan.name}</div>)} | |
| </div> | |
| ); | |
| } | |
| ``` | |
| import { useGetPlansQuery, useCreateDesignMutation } from "@meshery/schemas/dist/cloudApi"; | |
| function MyComponent() { | |
| const { data: plans, isLoading, error } = useGetPlansQuery(); | |
| if (isLoading) return <div>Loading...</div>; | |
| if (error) return <div>Error loading plans</div>; | |
| return ( | |
| <div> | |
| {plans?.map(plan => <div key={plan.id}>{plan.name}</div>)} | |
| </div> | |
| ); | |
| } |
CONTRIBUTING.md
Outdated
| make build # Run full build | ||
| npm run build # Build TypeScript distribution | ||
| go test ./... # Run Go tests | ||
| npx @redocly/cli lint schemas/constructs/v1beta1/model/api.yml # Lint OpenAPI |
There was a problem hiding this comment.
The commands listed for testing can be simplified. The make build target already executes npm run build (via build-ts) and go test ./... (via test-golang). Listing these commands separately is redundant and could be confusing. Simplifying this section will make the testing instructions more straightforward.
| make build # Run full build | |
| npm run build # Build TypeScript distribution | |
| go test ./... # Run Go tests | |
| npx @redocly/cli lint schemas/constructs/v1beta1/model/api.yml # Lint OpenAPI | |
| make build # Run full build, which includes tests | |
| npx @redocly/cli lint schemas/constructs/v1beta1/model/api.yml # Lint a specific OpenAPI file |
README.md
Outdated
| make setup && npm install # Install dependencies | ||
| make build # Generate Go, TypeScript, RTK Query | ||
| npm run build # Build TypeScript distribution |
There was a problem hiding this comment.
The Quick Start commands can be simplified. make setup already runs npm install, and make build includes the npm run build step. Providing redundant commands can be confusing for new contributors. Simplifying this will make the instructions clearer and more concise.
| make setup && npm install # Install dependencies | |
| make build # Generate Go, TypeScript, RTK Query | |
| npm run build # Build TypeScript distribution | |
| make setup # Install dependencies | |
| make build # Generate all code, build TS dist, and run tests |
1012Charan
force-pushed
the
docs/highlight-schema-driven-development
branch
3 times, most recently
from
9e21bec to
7257c68
Compare
1012Charan
changed the title
PragalvaXFREZ
left a comment
PragalvaXFREZ
left a comment
There was a problem hiding this comment.
Great work on the centralization! Left 2 comments on minor issues.
Holding off on approval until Lee's feedback about restoring/SKILLS is addressed.
| ### Schema File Roles | ||
|
|
||
| | File | Purpose | | ||
| |------|---------| | ||
| | `api.yml` | **Index file** - aggregates all subschemas via `$ref` and defines API endpoints for the construct | | ||
| | `<construct>.yaml` | **Subschema** - defines the main data model (noun) for the construct | | ||
| | `<construct>_core.yml` | **Subschema** - defines core/shared types used by the main schema | | ||
| | `templates/*.json` | **Templates** - example instances with default values | |
There was a problem hiding this comment.
The "Schema File Roles" table appears twice (also at lines 376-383). Remove the duplicate.
|
|
||
| - [GitHub Issues](https://github.com/meshery/schemas/issues) - Report bugs or request features | ||
| - [Community Slack](https://slack.meshery.io) - Real-time discussions with maintainers | ||
| - [Weekly Meetings](https://meshery.io/community/calendar) - Join our community calls |
There was a problem hiding this comment.
| - [Weekly Meetings](https://meshery.io/calendar) - Join our community calls |
broken link, needs to be changed
1012Charan
force-pushed
the
docs/highlight-schema-driven-development
branch
from
7257c68 to
ac39ca3
Compare
Signed-off-by: 1012Charan <charanvengala@gmail.com>
1012Charan
force-pushed
the
docs/highlight-schema-driven-development
branch
from
ac39ca3 to
f06e54f
Compare
|
Thanks for the review @PragalvaXFREZ , I have addressed the issues |
1012Charan
changed the title
|
Great work! LGTM. |
2 participants
Description
Centralizes contributing documentation and makes Schema-Driven Development (SDD) prominently visible across all documentation entry points. Also introduces a local skill to make SDD guidance accessible to AI agents.
Changes
1. New Skill for LLMs
2. Documentation Centralization
Fixes #571
Signed commits