Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Identify API + team to review it #2

Closed
kianenigma opened this issue May 1, 2023 · 5 comments · Fixed by paritytech/substrate#14115
Closed

Identify API + team to review it #2

kianenigma opened this issue May 1, 2023 · 5 comments · Fixed by paritytech/substrate#14115
Assignees
Labels
infra issue does not require specific knowledge of the content, and is mainly about the system.

Comments

@kianenigma
Copy link
Collaborator

kianenigma commented May 1, 2023

requirement for developers to identify if their PR is touching the application programming interface aka. API of our software, or our documentations (to the best of their knowledge) and if so:

  • pay extra attention to the code in that PR being well documented.
  • flag the PR (through either a request review, label, or similar) to be audited/reviewed by the those who maintain documentation.

For the time being, I think the easiest would be to use code-owners rules and assign a team to auto-review PRs if they touch a particular set of folders.

Might be a bit noisy, as this will be triggered for all non-draft PRs.

@hummusonrails
Copy link

@kianenigma Do you want me to try and set up an initial conversation with the Swimm team and see if their open source solution might be able to assist us with this? I think a lot of what their product does is exactly in this area.

@kianenigma
Copy link
Collaborator Author

Do you know if/how they can complement the above scenario?

Teams and actions are probably enough for now. I think we can set that up and try the whole process, and then see if we can improve it via Swimm.

I am kinda curious if the whole "DevRel audit" system works, and would like to set that up sooner rather than later.

@kianenigma
Copy link
Collaborator Author

kianenigma commented May 3, 2023

Given that I really want to kickstart this ASAP, here's my quick suggestion of how we can start on this:

The API

  • everything in frame, except
    • frame/support, because it is covered below.
    • frame/staking-bags-list-benchmarking
  • everything in frame/support/src
    • frame/support/procedural/lib.rs (where the macro-stubs are), the rest does not matter.
  • primitives/arithmetic
  • primitives/runtime
  • primitives/core

More candidates, but maybe skip for now:

  • primitives/api (important macros regarding runtime APIs live here).
  • primitives/io
  • primitives/weights
  • xcm
  • I presume there is a few sc-* crates could fit here as well, but I am not sure. @bkchr @paritytech/sdk-node any suggestions?

An alternative way to think about this, inspired by something that @athei once shared with me, would actually to create a single new create called frame-api. This crate will have ZERO code, and ONLY re-exports parts of substrate that you need to build a runtime with.

Whatever ends up being in this, ends up being well documented. It will also help us towards semver and so on.

I like the idea, but I am worried that it is me being too ambitious here and fixing too many things at once. Also, it is probably way more easier said than done.

  • Eventually, we can also consider adding a cfg macro to prevent missing_docs in these crates.

@ggwpez
Copy link
Member

ggwpez commented May 3, 2023

  • everything in frame, except

Might as well put them in the CODEOWNER file so the review request is automatic? Or ask CICD team for custom review GHA.

frame-api

Would be nice to have, since i think there are Rust tools that can detect breaking API changes - at least to function signature and returns. So even if this stays internal, could be useful to have it aggregated.

@kianenigma
Copy link
Collaborator Author

Might as well put them in the CODEOWNER file so the review request is automatic? Or ask CICD team for custom review GHA.

Initially CODEOWNERS should do, but later, if we see the system working we might need a custom review rule to eg. prevent merge.

@kianenigma kianenigma added the infra issue does not require specific knowledge of the content, and is mainly about the system. label May 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
infra issue does not require specific knowledge of the content, and is mainly about the system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants