Skip to content

MNT: Improve Contributing Guide#354

Merged
aladinor merged 7 commits intoopenradar:mainfrom
syedhamidali:maintainers
Apr 20, 2026
Merged

MNT: Improve Contributing Guide#354
aladinor merged 7 commits intoopenradar:mainfrom
syedhamidali:maintainers

Conversation

@syedhamidali
Copy link
Copy Markdown
Contributor

  • Changes are documented in history.md

Updates the contributing guide to clarify contributor, team-member, and maintainer roles, and adds a short pathway for getting more involved in xradar. See discussion in #341

Copy link
Copy Markdown
Collaborator

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Thanks @syedhamidali! Let's wait another day for others to chime in. But we can expand this anytime.

Comment thread docs/history.md Outdated
Comment thread docs/history.md Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.85%. Comparing base (6cba751) to head (ee8039a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #354   +/-   ##
=======================================
  Coverage   93.85%   93.85%           
=======================================
  Files          28       28           
  Lines        6165     6165           
=======================================
  Hits         5786     5786           
  Misses        379      379           
Flag Coverage Δ
notebooktests 0.00% <ø> (ø)
unittests 93.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aladinor
Copy link
Copy Markdown
Member

aladinor commented Apr 1, 2026

Thanks @syedhamidali for this PR.

I'd like to take this opportunity to open a broader discussion that I think is timely for all of us.

AI-Assisted Contributions

We all know that LLMs are now part of our development workflows. This is great — code can move faster than ever. But it also creates a real challenge for us as maintainers: reviewing and understanding contributions that can now be hundreds or thousands of lines, generated in minutes. The asymmetry between writing code with AI and reviewing it is significant, and we need to address it head-on.

The Icechunk team recently formalized an AI Usage Policy that I think captures this well. Their core principles:

  1. Contributor responsibility: If you submit a PR, you are responsible for understanding every line. You must be able to explain why each change is correct and how it fits into the project — regardless of whether the code was hand-written or AI-generated.
  2. Communication in your own words: PR descriptions, issue comments, and review responses must reflect your understanding, not pasted AI output.
  3. Open an issue first for large changes: Large AI-assisted contributions (refactors, new subsystems, migrations) should start with an issue to discuss scope and approach before implementation. This prevents contributors from shifting the review burden onto maintainers.
  4. Maintainers can close PRs where the scope makes meaningful review impractical or where the contributor cannot demonstrate understanding of their own changes.

This PR is a concrete example of how this plays out in practice — a 2,000+ line AI-generated contribution that prompted the Icechunk team to formalize their policy.
Or even this PR I am working on and I am using Claude code.

I think we should adopt a similar policy for xradar. We are a small team, and our review capacity is limited. Clear expectations will help both contributors and maintainers.

Proposed Process Improvements

Beyond the AI policy, I'd like to propose two things:

  1. Issue before PR: We should strongly encourage (or require) contributors to open an issue before starting implementation on non-trivial changes. The issue should summarize why the change is important, the proposed approach, and any downstream impacts. This gives maintainers a chance to provide early feedback and avoids wasted effort.

  2. Bi-weekly sync meeting: A 30-minute meeting every two weeks where we discuss open PRs, upcoming contributions, and project direction. This gives contributors space to present their ideas and ensures maintainers stay aligned on what's coming.

What do you think about this? @kmuehlbauer @mgrover1 @syedhamidali @egouden

@kmuehlbauer
Copy link
Copy Markdown
Collaborator

@aladinor Yes, + 💯 for AI policy as suggested. This topic would warrant it's own issue.

@kmuehlbauer kmuehlbauer mentioned this pull request Apr 17, 2026
6 tasks
@aladinor
Copy link
Copy Markdown
Member

@syedhamidali do you think we can use the the xarray IA policy as a template to merge this PR?

@syedhamidali
Copy link
Copy Markdown
Contributor Author

@aladinor absolutely, it looks great.

@kmuehlbauer
Copy link
Copy Markdown
Collaborator

@syedhamidali @aladinor What's the plan to move on here? Should this be merged as-is or will the mentioned AI policy added here, too?

@aladinor
Copy link
Copy Markdown
Member

aladinor commented Apr 20, 2026

@kmuehlbauer let me see if I can adapt the IA policy today along with #361 in a new PR

@aladinor aladinor merged commit 464b4dc into openradar:main Apr 20, 2026
11 checks passed
@zssherman
Copy link
Copy Markdown

zssherman commented Apr 20, 2026

Hello @aladinor @kmuehlbauer and folks! Figure to post here to discuss what we discussed @scollis @rcjackson @jrobrien91 plan on doing for Py-ART as well. I do like your points on the AI policy, funny enough we were discussing a similar policy for Py-ART utilizing, the xarray policy as a starting point. The only other points we might be adding is that for CI related PRs for actions or dependencies or if a person/agent is trying to add a dependency to the environment that AI is advised not to be used or maintainer only or an issue is opened to disuss the new dependency etc. The thought behind this is there have been malicious cases or PRs trying to add malicious dependencies, which can be a concern. And also if users, if they do use AI to possibly list what model and version. Were willing to discuss more if need be! But figure to share our thoughts as well. Cheers!

@aladinor
Copy link
Copy Markdown
Member

Thanks @zssherman for the thoughtful additions. Both of your points made it into a new draft PR I just opened to continue this conversation with the wider community: #363.

What's there:

  • Policy adapted from xarray (with attribution), mirroring its structure.
  • A dedicated CI, Packaging, and Dependency Changes section covering GitHub Actions, dependency adds/bumps, pyproject.toml / environment.yml / pre-commit config, and security-sensitive areas — all requiring an issue-first discussion before AI-assisted changes, explicitly because of the supply-chain concern you raised.
  • A Disclosing AI Usage section that recommends noting the tool and model/version (e.g. "Claude Opus 4.7") in the PR description.

It's opened as draft so we can iterate on the language before merge — feedback from you, @scollis, @rcjackson, and @jrobrien91 is very welcome, especially if Py-ART ends up with a similar policy and we can keep them aligned.

Thanks @kmuehlbauer @syedhamidali @mgrover1 @egouden — please chime in on #363 when you have a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants