Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CONTRIBUTING.md #1010

Open
stan-dot opened this issue Jan 24, 2025 · 2 comments
Open

Make CONTRIBUTING.md #1010

stan-dot opened this issue Jan 24, 2025 · 2 comments

Comments

@stan-dot
Copy link
Contributor

From discussion with @DiamondJoseph ; there are non-trivial acceptance criteria to PRs that could be more explicit.

  • does device connect - on one beamline, or all?
  • when do we allow 'skip_device' in a PR?
    should PR code correspond to the beamline reality at merge OR to the model of beamline reality and the exact details could be worked in a separate PR?
@stan-dot
Copy link
Contributor Author

current 'contributing' documentation is not as plain to see and does not cover those scenarios
https://diamondlightsource.github.io/dodal/main/how-to/contribute.html

@callumforrester
Copy link
Contributor

While contributing guidelines are useful I think we should be careful.

Devices may fail to connect due to temporary beamline network issues, shutdowns, power cuts, or just being occasional-use. None of these should block a Github PR from being merged. Between these I do not think there will ever be a single instant where every device on device beamline connects, so there's little point in requiring that.

beamline reality at merge OR to the model of beamline reality and the exact details could be worked in a separate PR?

This one harder but personally I'm against formalizing this too. @stan-dot I think the docs you actually want are here. We are trialling the approved reviewer system in the hope that these kinds of decisions will spread culturally rather than formally. That means the people in the review team have a lot of weight in the decision. Personally I would bias myself towards beamline reality at merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants