docs(contribution-guidelines): add best practices section for coding guidelines #112
Conversation
❌ Deploy Preview for cuhacking-portal-dev failed. Why did it fail? →
|
|
Warning Rate limit exceeded@MFarabi619 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a comprehensive "Best Practices" section to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (3)
99-99: Remove redundant phrase.As suggested by the static analysis tool, use "inside" instead of "inside of" to avoid redundancy and improve clarity.
Apply this diff to fix the issue:
-All E2E tests located inside of a directory(Organized based on what overall page/tool the tests are using) +All E2E tests are located inside a directory (organized based on the overall page/tool the tests are using).Tools
LanguageTool
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...(OUTSIDE_OF)
100-100: Remove redundant phrase.As suggested by the static analysis tool, use "inside" instead of "inside of" to avoid redundancy and improve clarity.
Apply this diff to fix the issue:
-- For example, all E2E tests relating to the cuhacking portal are stored inside of the portal--e2e directory. +- For example, all E2E tests related to the cuhacking portal are stored inside the `portal-e2e` directory.Tools
LanguageTool
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...(OUTSIDE_OF)
140-140: Rephrase the sentence for clarity.As suggested by the static analysis tool, consider rephrasing the sentence to strengthen the wording and improve clarity.
Apply this diff to rephrase the sentence:
-- Report is produced regardless of whether you push code that makes changes to these E2E tests. +- The report is generated even if the pushed code does not modify the E2E tests.Tools
LanguageTool
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...(MAKE_CHANGES)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (1 hunks)
Additional context used
LanguageTool
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...Testing* #### E2E Test catagorization End to end Testing broken down into 2 different pa...(END_TO_END_HYPHEN)
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...(OUTSIDE_OF)
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...(OUTSIDE_OF)
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...(MAKE_CHANGES)
Additional comments not posted (6)
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (6)
88-88: Great addition!The "Best Practices" section is a valuable addition to the coding guidelines documentation. It will provide clear guidance to contributors and help maintain consistency across the codebase.
105-105: Informative note about theplaywright.configfile.The information about using a
playwright.configfile for managing tests across different environments is helpful for contributors. It provides clarity on how to handle tests for various environments.
109-109: Appropriate subsection title.The "Creating E2E tests" subsection title is clear and relevant to the content that follows. It provides a logical structure to the best practices section.
123-123: Clear naming convention guideline.The "Naming conventions" subsection title is appropriate, and the guideline for constants (all caps with words separated by underscores) is a common and widely accepted practice. It helps maintain consistency in the codebase.
126-126: Helpful subsection on running tests.The "Running tests" subsection title is appropriate, and providing information about the commands to run tests is helpful for contributors. It facilitates the testing process and ensures consistency.
137-137: Informative subsection on viewing tests.The "Viewing tests" subsection title is appropriate, and mentioning that all tests can be viewed on the GitHub workflow is helpful for contributors. It provides clarity on where to find test results and reports.
|
|
||
| ## Best Practices | ||
| ### **Testing** | ||
| #### E2E Test catagorization |
There was a problem hiding this comment.
Fix typo and use hyphenated form.
- Please fix the typo in the subsection title. It should be "categorization" instead of "catagorization".
- As suggested by the static analysis tool, use the hyphenated form "End-to-end" instead of "End to end" for consistency and clarity.
Apply this diff to fix the issues:
-#### E2E Test catagorization
-End to end Testing broken down into 2 different parts
+#### End-to-End Test Categorization
+End-to-end testing is broken down into 2 different parts: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.
| #### E2E Test catagorization | |
| #### End-to-End Test Categorization | |
| End-to-end testing is broken down into 2 different parts: |
Tools
LanguageTool
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...Testing* #### E2E Test catagorization End to end Testing broken down into 2 different pa...(END_TO_END_HYPHEN)
✅ Deploy Preview for website-cuhacking ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (2)
144-155: LGTM: Backend guidelines provide valuable advice.The Backend subsection offers useful guidance on commit descriptions, defining requirements, and cautions about modifying tsconfig files. This aligns well with good development practices.
Consider providing more specific examples or consequences of frequent tsconfig modifications to help developers understand the potential impact better. For example:
Be cautious when modifying tsconfig files. -Frequent modifications to tsconfig files can lead to a snowball effect when adding features - or modifying existing features. +Frequent modifications to tsconfig files can lead to unintended consequences: + - Compilation errors in seemingly unrelated parts of the codebase + - Inconsistent behavior between local development and CI/CD environments + - Difficulties in maintaining consistent code quality across the project
158-163: Consider improving or removing TBD sections.The "Features" and "API" subsections are currently marked as TBD (To Be Determined). While it's good to have placeholders for these important topics, they don't provide value to contributors in their current state.
Consider one of the following approaches:
- Remove these sections until content is available.
- Provide a brief explanation of what these sections will cover in the future and when contributors can expect the content to be added.
For example:
### Features This section will cover best practices for feature development, including: - Feature flagging - A/B testing - Performance considerations (Coming soon) ### API This section will provide guidelines for API design and development, including: - RESTful principles - Authentication and authorization - Versioning (Coming soon)This gives contributors an idea of what to expect and shows that these topics are planned for future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...Testing* #### E2E Test catagorization End to end Testing broken down into 2 different pa...(END_TO_END_HYPHEN)
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...(OUTSIDE_OF)
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...(OUTSIDE_OF)
[uncategorized] ~140-~140: Possible missing article found.
Context: ... PR - Can get the report from here. - Report is produced regardless of whether you p...(AI_HYDRA_LEO_MISSING_THE)
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...(MAKE_CHANGES)
🔇 Additional comments (2)
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (2)
87-87: LGTM: New "Best Practices" section added.The addition of a "Best Practices" section aligns well with the PR objectives to enhance the coding guidelines documentation. This will provide valuable guidance for contributors.
87-173: Overall assessment: Significant improvements to coding guidelinesThe additions to the coding guidelines document are a substantial step forward in providing clear and comprehensive guidance for contributors. The new "Best Practices" section, particularly the detailed "Testing" subsection, offers valuable insights that will help maintain code quality and consistency across the project.
Key points:
- The "Testing" subsection provides thorough guidance on E2E testing practices.
- The "Backend" subsection offers useful advice on commit descriptions and requirement definitions.
- Placeholder sections for "Features," "API," and "Design" indicate areas for future expansion.
While there are some minor issues to address (typos, formatting, and empty sections), the overall direction and content of these changes are positive and align well with the PR objectives.
To further improve this PR:
- Address the formatting and typo issues in the "Testing" subsection.
- Consider expanding or removing the TBD and empty sections as suggested in previous comments.
- Review the document for any additional opportunities to enhance clarity or provide examples where appropriate.
Great work on expanding these guidelines! These changes will significantly benefit contributors to the project.
🧰 Tools
🪛 LanguageTool
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...Testing* #### E2E Test catagorization End to end Testing broken down into 2 different pa...(END_TO_END_HYPHEN)
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...(OUTSIDE_OF)
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...(OUTSIDE_OF)
[uncategorized] ~140-~140: Possible missing article found.
Context: ... PR - Can get the report from here. - Report is produced regardless of whether you p...(AI_HYDRA_LEO_MISSING_THE)
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...(MAKE_CHANGES)
| ### **Testing** | ||
| #### E2E Test catagorization | ||
| End to end Testing broken down into 2 different parts | ||
| - Desktop | ||
| - Mobile | ||
| - Tablet | ||
| These above tests are broken down into further subsections: | ||
| - Mobile and tablet | ||
| - Desktop tabet | ||
| - Desktop | ||
| #### E2E test organization | ||
| All E2E tests located inside of a directory(Organized based on what overall page/tool the tests are using) | ||
| - For example, all E2E tests relating to the cuhacking portal are stored inside of the portal--e2e directory. | ||
| - All related tests are stored in a .ts file. | ||
| - For example, all tests related to the docs pages are stored inside one .ts file | ||
| - There may be other files that contain helper functions for these tests. | ||
| - These helper functions are stored in a dedicated | ||
| For tests involving different environments, a playwright.config file is used. | ||
|
|
||
| Once you think that all the tests are good to go, you can run the code using playwright or in visual studio code. | ||
|
|
||
| #### Creating E2E tests | ||
| Using code gen to generate the difference events that happen in the docs. | ||
|
|
||
| General structure of the tests: | ||
|
|
||
| `` | ||
| test('name", async ((.docsLayoutPage))) => ({ | ||
| events_from_codegen | ||
| }) | ||
| `` | ||
|
|
||
| pom.ts is used to create specific variables to represent different elements in the page. | ||
|
|
||
| #### Naming conventions | ||
| Constants are all caps with words being separated by underscores. | ||
|
|
||
| #### Running tests | ||
| 2 commands to run tests: | ||
| For running the tests through cmd line->1. `pnpm nx run docs-e2e:e2e --verbose` | ||
|
|
||
| For running this command to run codegen event generator->npx playwright codegen http:localhost:3000/docs | ||
|
|
||
| For different tests, indicate DESKTOP, MOBILE, TABLET depending on the environment | ||
|
|
||
| If you run a bunch of tests at the same time, they might time out | ||
| It is advised to run each test individually | ||
|
|
||
| #### Viewing tests | ||
| All tests can be viewed on the github workflow. | ||
| - Can access them in the PR | ||
| - Can get the report from here. | ||
| - Report is produced regardless of whether you push code that makes changes to these E2E tests. | ||
|
|
||
| When running different tests, make sure you select the right environments for each of these tests. | ||
|
|
There was a problem hiding this comment.
Improve formatting and fix typo in the Testing subsection.
The Testing subsection provides valuable guidance on E2E testing practices. However, there are a few areas that could be improved:
- Fix the typo in "catagorization" (should be "categorization") at line 89.
- Use the hyphenated form "End-to-end" instead of "End to end" for consistency.
- Improve formatting for better readability, especially in code blocks and lists.
Here are some suggested changes:
-#### E2E Test catagorization
-End to end Testing broken down into 2 different parts
+#### E2E Test Categorization
+End-to-end testing is broken down into 2 different parts:For code blocks, use triple backticks (```) with the language specified for syntax highlighting. For example:
-``
-test('name", async ((.docsLayoutPage))) => ({
- events_from_codegen
-})
-``
+```typescript
+test('name', async ({ docsLayoutPage }) => {
+ // events_from_codegen
+});
+```
Consider using more consistent formatting for lists and indentation throughout the section to improve readability.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 LanguageTool</summary><blockquote>
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...*Testing** #### E2E Test catagorization End to end Testing broken down into 2 different pa...
(END_TO_END_HYPHEN)
---
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...
(OUTSIDE_OF)
---
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...
(OUTSIDE_OF)
---
[uncategorized] ~140-~140: Possible missing article found.
Context: ... PR - Can get the report from here. - Report is produced regardless of whether you p...
(AI_HYDRA_LEO_MISSING_THE)
---
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...
(MAKE_CHANGES)
</blockquote></details>
</details>
<!-- This is an auto-generated comment by CodeRabbit -->| ### **Design** | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Address the empty "Design" subsection.
The "Design" subsection is currently empty. This doesn't provide any value to contributors and may cause confusion.
Consider one of the following actions:
- If the section is not needed at this time, remove it entirely.
- If it's intended for future content, add a brief description of what will be covered, similar to the suggestion for the "Features" and "API" sections.
- If it's ready to be populated, add some initial content or guidelines related to design practices in the project.
For example, if keeping the section for future content:
### Design
This section will cover best practices for design in our project, including:
- UI/UX principles
- Accessibility guidelines
- Design system usage
(Coming soon)This approach provides context for the section's future content while making it clear that it's a work in progress.
There was a problem hiding this comment.
@HasithDeAlwis, understood. Let me know if you need any further assistance.
HasithDeAlwis
force-pushed
the
ashwin/docs/104-add-best-practices-as-a-subsection
branch
from
d5854d3
to
711b536
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (2)
144-156: Enhance clarity and consistency in the Backend subsection.The Backend subsection provides valuable guidance. Consider the following suggestions to improve clarity and consistency:
- Rephrase the opening sentence for clarity:
-When working on an issue, making commits with description when necessary. +When working on an issue, make descriptive commits as necessary.
- Use consistent formatting for bullet points:
- For example, when generating a library(ex: a database library) that will be used, - indicate the command used to generate that library in the commit description. + indicate the command used to generate that library in the commit description. - Additionally, when working on an issue, begin with defining the requirements to a given issue - before attempting to resolve it. + before attempting to resolve it.
- Improve the clarity of the caution about tsconfig files:
Be cautious when modifying tsconfig files. -- Frequent modifications to tsconfig files can lead to a snowball effect when adding features - or modifying existing features. +- Frequent modifications to tsconfig files can lead to a snowball effect when adding or + modifying features, potentially causing unexpected issues throughout the project.
158-160: Enhance the Features subsection with a brief description.While it's understandable that the content for this section is still to be determined, providing a brief description or outline of what will be covered can be helpful for contributors. Consider adding a short explanation of what kind of information will be included in this section in the future.
For example:
### **Features** This section will cover best practices for feature development, including: - Feature request process - Implementation guidelines - Feature testing and validation - Documentation requirements (Content to be added soon)This approach gives context for the section's future content while making it clear that it's a work in progress.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...Testing* #### E2E Test catagorization End to end Testing broken down into 2 different pa...(END_TO_END_HYPHEN)
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...(OUTSIDE_OF)
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...(OUTSIDE_OF)
[uncategorized] ~140-~140: Possible missing article found.
Context: ... PR - Can get the report from here. - Report is produced regardless of whether you p...(AI_HYDRA_LEO_MISSING_THE)
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...(MAKE_CHANGES)
🔇 Additional comments (2)
apps/docs/src/content/docs/contribution-guidelines/coding-guidelines.mdx (2)
162-173: Improve the API and Design subsections.
- For the API subsection:
Similar to the suggestion for the Features section, consider adding a brief description of what will be covered in the API section. For example:### **API** This section will cover best practices for API development, including: - API design principles - Documentation standards - Versioning guidelines - Security considerations (Content to be added soon)
- For the Design subsection:
The Design subsection is currently empty. Consider one of the following actions:a. If the section is not needed at this time, remove it entirely.
b. If it's intended for future content, add a brief description of what will be covered, similar to the suggestion for the API section.
c. If it's ready to be populated, add some initial content or guidelines related to design practices in the project.For example, if keeping the section for future content:
### **Design** This section will cover best practices for design in our project, including: - UI/UX principles - Accessibility guidelines - Design system usage (Content to be added soon)
88-143:⚠️ Potential issueImprove formatting and fix remaining issues in the Testing subsection.
Thank you for adding this comprehensive Testing subsection. There are a few remaining issues that need to be addressed:
- Use consistent formatting for lists and subheadings throughout the section.
- Fix typos and grammatical errors.
- Improve code block formatting.
Here are some suggested changes:
- For the E2E Test categorization:
-#### E2E Test catagorization -End to end Testing broken down into 2 different parts +#### E2E Test Categorization +End-to-end testing is broken down into 3 different parts: - Desktop - Mobile - Tablet -These above tests are broken down into further subsections: +These tests are further categorized into: - Mobile and tablet - - Desktop tabet + - Desktop tablet - Desktop
- For the code block:
-`` -test('name", async ((.docsLayoutPage))) => ({ - events_from_codegen -}) -`` +```typescript +test('name', async ({ docsLayoutPage }) => { + // events_from_codegen +}); +``` 3. Use consistent formatting for command examples: ```diff -For running the tests through cmd line->1. `pnpm nx run docs-e2e:e2e --verbose` +For running the tests through command line: +1. `pnpm nx run docs-e2e:e2e --verbose` -For running this command to run codegen event generator->npx playwright codegen http:localhost:3000/docs +2. To run the codegen event generator: + `npx playwright codegen http://localhost:3000/docs`
- Improve clarity and grammar:
-When running different tests, make sure you select the right environments for each of these tests. +When running different tests, make sure to select the appropriate environment for each test.🧰 Tools
🪛 LanguageTool
[grammar] ~89-~89: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...Testing* #### E2E Test catagorization End to end Testing broken down into 2 different pa...(END_TO_END_HYPHEN)
[style] ~99-~99: This phrase is redundant. Consider using “inside”.
Context: ...test organization All E2E tests located inside of a directory(Organized based on what ove...(OUTSIDE_OF)
[style] ~100-~100: This phrase is redundant. Consider using “inside”.
Context: ...ting to the cuhacking portal are stored inside of the portal--e2e directory. - All rela...(OUTSIDE_OF)
[uncategorized] ~140-~140: Possible missing article found.
Context: ... PR - Can get the report from here. - Report is produced regardless of whether you p...(AI_HYDRA_LEO_MISSING_THE)
[style] ~140-~140: Consider shortening or rephrasing this to strengthen your wording.
Context: ...egardless of whether you push code that makes changes to these E2E tests. When running differe...(MAKE_CHANGES)
There was a problem hiding this comment.
Content is ok, but these should be their own directories/pages. Can you refer to how Farabi did his, here
| ### **Design** | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
JeremyFriesenGitHub
added
good first issue
|
Closing as stale |
…ed the sections Features, API, and Design.
MFarabi619
force-pushed
the
ashwin/docs/104-add-best-practices-as-a-subsection
branch
from
711b536
to
1b3dcb7
Compare
✅ Deploy Preview for docs-cuhacking ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
🚩 Linked issue
The issue is 104. The issue can be found here..
🛠 How does this PR address the issue(s)? How does it solve the problem?
The following PR is for adding a best practices section to the coding guidelines page. Best practices would describe to new contributors conventions for different areas within cuHacking's development infrastructure.
These areas are:
✅ Checklist
Make sure your PR follows the according checklist:
Code Quality
PR Review Readiness
Summary by CodeRabbit
New Features
Documentation