LCORE-663: Updated contributing guide #532
Conversation
WalkthroughReorganizes documentation: CONTRIBUTING.md’s sections are restructured (moving Code coverage and Linters to top-level; splitting Linters into Ruff and Pylint; adding a Formatting rules subsection under Code style; adjusting TOC). Adds a note in docs/testing.md advising using pytest exclusively and not mixing pytest and unittest. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches🧪 Generate unit tests
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
139-155: Invalid Ruff CLI command; replace with correct guidance.
ruff linteris not a valid subcommand. Recommend pointing to docs and usingruff rule <CODE>orruff --explain <CODE>.-List of all _Ruff_ rules recognized by Ruff can be retrieved by: - -``` -ruff linter -``` - -Description of all _Ruff_ rules are available on https://docs.astral.sh/ruff/rules/ - -Ruff rules can be disabled in source code (for given line or block) by using special `noqa` comment line. For example: +See the complete list of rules: https://docs.astral.sh/ruff/rules/ + +Explain a specific rule from the CLI: +``` +ruff rule E402 +# or +ruff --explain E402 +``` + +Disable a rule inline with `noqa`, for example:
🧹 Nitpick comments (3)
CONTRIBUTING.md (3)
14-22: Fix TOC nesting to satisfy markdownlint MD007 (2-space indents).Current nested bullets use 4 spaces; expected is 2.
-* [Linters](#linters) - * [Type hints checks](#type-hints-checks) - * [Ruff](#ruff) - * [Pylint](#pylint) +* [Linters](#linters) + * [Type hints checks](#type-hints-checks) + * [Ruff](#ruff) + * [Pylint](#pylint) * [Code style](#code-style) - * [Formatting rules](#formatting-rules) + * [Formatting rules](#formatting-rules)
123-124: Grammar fix and tool name formatting.Minor clarity/readability improvements.
-_Black_, _Ruff_, Pyright, _Pylint_, __Pydocstyle__, __Mypy__, and __Bandit__ tools are used as linters. There are a bunch of linter rules enabled for this repository. All of them are specified in `pyproject.toml`, such us in sections `[tool.ruff]` and `[tool.pylint."MESSAGES CONTROL"]`. Some specific rules can be disabled using `ignore` parameter (empty now). +_Black_, _Ruff_, _Pyright_, _Pylint_, _pydocstyle_, _mypy_, and _Bandit_ are used. Linter rules are configured in `pyproject.toml` (for example, `[tool.ruff]` and `[tool.pylint."MESSAGES CONTROL"]`). Some rules can be disabled using the `ignore` parameter (currently empty).
186-189: Optional: mention formatter invocation for consistency.If Black is the sole formatter, reference the existing Make target to reduce ambiguity.
-Code formatting rules are checked by __Black__. More info can be found on [https://black.readthedocs.io/en/stable/](https://black.readthedocs.io/en/stable/). +Code formatting is enforced by Black. Run `make format` (or via pre-commit). More info: https://black.readthedocs.io/en/stable/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CONTRIBUTING.md(5 hunks)docs/testing.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
| ## Code coverage measurement | ||
|
|
||
| During testing, code coverage is measured. If the coverage is below defined threshold (see `pyproject.toml` settings for actual value stored in section `[tool.coverage.report]`), tests will fail. We measured and checked code coverage in order to be able to develop software with high quality. | ||
|
|
||
| Code coverage reports are generated in JSON and also in format compatible with [_JUnit_ test automation framework](https://junit.org/junit5/). It is also possible to start `make coverage-report` to generate code coverage reports in the form of interactive HTML pages. These pages are stored in the `htmlcov` subdirectory. Just open the index page from this subdirectory in your web browser. | ||
|
|
||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify coverage report formats match what CI actually produces.
Text states “JSON and JUnit-compatible” coverage outputs. Coverage.py typically emits XML/HTML; JUnit is usually for test results, not coverage.
Consider aligning this with the Makefile/CI. To check:
🏁 Script executed:
#!/bin/bash
# Inspect coverage targets and report outputs
rg -nC2 -i 'coverage|cov' Makefile **/.github/workflows/*.yml **/pyproject.tomlLength of output: 1024
Clarify coverage vs. JUnit outputs.
Makefile shows coverage is emitted as JSON (--cov-report "json:...") and pytest writes JUnit XML as test-result files (--junit-xml). CONTRIBUTING.md (lines 113–119) is misleading — JUnit XML is not a coverage report. Update CONTRIBUTING.md to state coverage reports are JSON and that test results are produced in JUnit XML, and either add/point to pyproject.toml if it contains the coverage threshold or remove/replace that reference (Makefile unit/integration targets are the authoritative source).
🤖 Prompt for AI Agents
In CONTRIBUTING.md around lines 113–119, the text incorrectly implies JUnit XML
is a coverage report; update the paragraph to state that coverage reports are
generated as JSON (and HTML via make coverage-report/htmlcov) and that pytest
writes JUnit XML files containing test results (not coverage), and then either
point readers to pyproject.toml for the coverage threshold or remove that
reference and instead point to the Makefile (unit/integration targets) as the
authoritative source for how coverage and test result outputs are produced; make
the wording concise and replace the misleading sentence accordingly.
| [NOTE] Please do not mix pytest and unittest modules in new tests. It is preferred to use pytest in every tests, which will help the team to maintain consistency. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify pytest vs unittest guidance; fix grammar.
Current note conflicts with earlier guidance that unittest is used for mocking/patching and has grammar issues.
Apply:
-[NOTE] Please do not mix pytest and unittest modules in new tests. It is preferred to use pytest in every tests, which will help the team to maintain consistency.
+[NOTE] Prefer pytest for all new tests and avoid writing unittest.TestCase-based tests. Using unittest.mock within pytest tests is fine. This consistency helps the team maintain the suite.📝 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.
| [NOTE] Please do not mix pytest and unittest modules in new tests. It is preferred to use pytest in every tests, which will help the team to maintain consistency. | |
| [NOTE] Prefer pytest for all new tests and avoid writing unittest.TestCase-based tests. Using unittest.mock within pytest tests is fine. This consistency helps the team maintain the suite. |
🤖 Prompt for AI Agents
In docs/testing.md around lines 72 to 73, the current note both has grammar
problems and conflicts with earlier guidance about using unittest for
mocking/patching; replace it with a clear, grammatically-correct sentence that
says: prefer writing tests with pytest for consistency, do not mix pytest and
unittest frameworks within the same test file, and it is acceptable to use
unittest.mock (or the mock library) for mocking/patching when needed. Ensure the
wording is concise and consistent with the earlier section about mocking.
1 participant
Description
LCORE-663: Updated contributing guide
Type of change
Related Tickets & Documents
Summary by CodeRabbit