Update ADIT Client link & add documentation link to readme #297
Update ADIT Client link & add documentation link to readme #297mhumzaarain merged 5 commits intomainfrom
Conversation
Summary of ChangesHello @mhumzaarain, 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 primarily focuses on maintaining up-to-date documentation and configuration. It corrects an outdated link for the ADIT Client to ensure users are directed to the correct resource on PyPI, and streamlines the MkDocs configuration by removing an unnecessary copyright declaration. Highlights
Changelog
Activity
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
|
📝 WalkthroughWalkthroughAdds a new MkDocs hook module that injects a dynamic copyright year, updates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 updates the link for the ADIT Client in the README and removes the copyright information from the mkdocs.yml configuration. While updating the link is a good improvement, I've suggested using a version-agnostic URL to avoid it becoming outdated. More importantly, the removal of the copyright notice is a significant change that should be carefully considered and documented, as it has potential legal implications.
| repo_url: https://github.com/openradx/adit | ||
| edit_uri: edit/main/docs/ | ||
|
|
||
| copyright: Copyright © 2024-2025 CCI Bonn |
There was a problem hiding this comment.
This change removes the copyright notice from the documentation website's configuration. This is a significant change with potential legal implications that is not mentioned in the pull request description. Copyright information is crucial for defining ownership and licensing for the project. This removal should be reverted unless it is intentional and approved, in which case it should be explicitly justified in the PR description.
| ## API Client | ||
|
|
||
| [ADIT Client](https://github.com/openradx/adit-client) is a Python library to query, retrieve and upload DICOM images programmatically from a Python script. Thereby it can interact with DICOM (e.g. PACS) servers connected to an ADIT server. | ||
| [ADIT Client](https://pypi.org/project/adit-client/0.5.0/) is a Python library to query, retrieve and upload DICOM images programmatically from a Python script. Thereby it can interact with DICOM (e.g. PACS) servers connected to an ADIT server. |
There was a problem hiding this comment.
Linking to a specific version of the adit-client on PyPI can cause the documentation to become outdated quickly. It's generally better to link to the main project page, which always shows the latest version.
| [ADIT Client](https://pypi.org/project/adit-client/0.5.0/) is a Python library to query, retrieve and upload DICOM images programmatically from a Python script. Thereby it can interact with DICOM (e.g. PACS) servers connected to an ADIT server. | |
| [ADIT Client](https://pypi.org/project/adit-client/) is a Python library to query, retrieve and upload DICOM images programmatically from a Python script. Thereby it can interact with DICOM (e.g. PACS) servers connected to an ADIT server. |
PR Review: Update ADIT Client LinkThank you for the contribution! I've reviewed the changes and have some feedback: Code Quality & Best PracticesREADME.md - Link Change: Issues:
Recommendation:
mkdocs.yml - Copyright Removal:
Concerns
Suggestions
Security & PerformanceNo security or performance concerns - these are documentation-only changes. Would you like to discuss these suggestions? |
PR Review: Update ADIT Client linkSummaryThis PR updates the ADIT Client reference in README.md to point to the PyPI package page instead of a GitHub repository link, and removes the copyright field from mkdocs.yml. Code Quality ✅Strengths:
Changes Analysis1. README.md Link Update (Line 80)Change: Assessment: ✅ Good decision
Note: The GitHub repository still exists at 2. mkdocs.yml Copyright Removal (Lines 9-10)Change: Removed Consideration:
Question for maintainers: Was this removal intentional, or should it be a separate PR with proper justification? Potential IssuesMissing Updates in DocumentationThe documentation files also reference the ADIT Client but weren't updated:
Recommendation: Consider adding PyPI links to these documentation files for consistency: The **[ADIT Client](https://pypi.org/project/adit-client/)** is a Python package...adit-client/README.md ReferenceThe client's own README ( Testing ✅No functional code changes - documentation only. Manual verification of the PyPI link would be sufficient. Security ✅No security concerns. External links are to trusted domains (pypi.org). Best PracticesCommit messages: Both commits have clear, descriptive messages that explain the changes well. Scope: The first commit mixes two unrelated changes (link update + copyright removal). Best practice would be to separate these into distinct commits. Recommendations
Overall Assessment: ✅ Approve with Minor SuggestionsThe core change (updating the README link to PyPI) is correct and beneficial. The copyright removal warrants clarification but doesn't block the PR. The documentation files could be updated in a follow-up PR for consistency. Suggested follow-up:
|
…on link to readme
PR ReviewThis is a small documentation-only PR. Here's my feedback: Summary of Changes
FeedbackREADME.md — documentation link placement README.md — PyPI vs. GitHub link for ADIT Client [ADIT Client](https://pypi.org/project/adit-client/) ([source](https://github.com/openradx/adit-client))docs/hooks/copyright.py — dynamic year is a good improvement docs/hooks/copyright.py — HTML entity in Python string mkdocs.yml — hardcoded start year VerdictThe changes are correct and improve the docs. No functional code is affected. The PR is safe to merge. The suggestions above are minor improvements, not blockers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/hooks/copyright.py (1)
1-6: Clean and correct hook implementation.Minor observations:
- If the build runs in 2024, the copyright renders as "2024-2024". You could conditionally collapse to a single year, but this is purely cosmetic.
- Adding a type annotation for the
configparameter would satisfy pyright and align with the Google Python Style Guide.♻️ Optional refinement
-from datetime import datetime +from datetime import datetime + +from mkdocs.config.defaults import MkDocsConfig -def on_config(config): - config["copyright"] = f"Copyright © 2024-{datetime.now().year} CCI Bonn" +def on_config(config: MkDocsConfig) -> MkDocsConfig: + current_year = datetime.now().year + start_year = 2024 + year_range = str(start_year) if current_year == start_year else f"{start_year}-{current_year}" + config["copyright"] = f"Copyright © {year_range} CCI Bonn" return config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/hooks/copyright.py` around lines 1 - 6, The on_config hook sets a year range unconditionally and lacks a type annotation; update on_config(config) to accept a typed mapping (e.g., dict[str, Any] or MutableMapping) and compute current_year = datetime.now().year then set config["copyright"] to either "Copyright © 2024 CCI Bonn" when current_year == 2024 or "Copyright © 2024-{current_year} CCI Bonn" otherwise, ensuring the value uses the formatted string and return the annotated config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/hooks/copyright.py`:
- Around line 1-6: The on_config hook sets a year range unconditionally and
lacks a type annotation; update on_config(config) to accept a typed mapping
(e.g., dict[str, Any] or MutableMapping) and compute current_year =
datetime.now().year then set config["copyright"] to either "Copyright © 2024 CCI
Bonn" when current_year == 2024 or "Copyright © 2024-{current_year} CCI Bonn"
otherwise, ensuring the value uses the formatted string and return the annotated
config.
mhumzaarain
changed the title
PR Review: Update ADIT Client link & add documentation link to readmeThis is a small, focused documentation PR with three changes. Overall the changes are reasonable, but I have a few observations.
|
| Area | Status |
|---|---|
| Documentation link in README | Good addition |
Section rename (API Client → ADIT Client) |
Appropriate |
| PyPI vs GitHub link for ADIT Client | Worth discussing — consider keeping or adding back the GitHub link |
| Copyright hook | Correct pattern; minor nit on magic start year |
| Tests | N/A (documentation-only change) |
| Security | No concerns |
PR ReviewThis is a small, focused documentation improvement PR. Overall it's clean and reasonable. A few observations: README.md
|
1 participant
adit-clientlink issueread me