Conversation
✅ Deploy Preview for openapi-ts canceled.
|
|
size-limit report 📦
|
gzm0
left a comment
gzm0
left a comment
There was a problem hiding this comment.
TY for writing this. This LGTM modulo the bit about "one test" (but it feels we're in agreement and it's mostly wordsmithing).
| Keeping the North Star in mind, here’s the necessary criteria for a PR: | ||
|
|
||
| - **Tests written.** No exceptions. They make a change, they MUST write a test. | ||
| - Updating existing tests is not OK. It must be a new test (if their change does necessitate updating other tests, that’s fine but doesn’t count towards testing their PR, but also, updating existing tests falls under breaking change consideration) |
There was a problem hiding this comment.
I feel that's a bit too low-level of a criteria? I fully agree with the underlying sentiment (IIUC: new stuff must be appropriately tested, bugfixes must have regression tests).
Sometimes, I could see how extending an existing test is a better / more readable way of testing a change. (Maybe I'm nit-picking too much on the term "a test" here 🤷 ).
| - It might be a `major` release if the example changes are thousands and thousands of lines (and the changes are complex and layered). | ||
| - It might be a `major` release if the output seems laterally-different, such as replacing `|` for `&`. | ||
|
|
||
| There’s more subjectivity to it than one realizes. In general, try and estimate how many people _may_ be impacted. Use the OpenAPI example specs as a reference—you can at least see how much it changes the output for a handful of real-world scenarios. Try and wait to ship sweeping, high-impact changes for major releases if at all possible. |
There was a problem hiding this comment.
It might make sense going forward to narrow down our guidelines w.r.t. this. For example: is a change requiring a newer typescript feature but clearly having better types a breaking change?
The main reason I think we should narrow down guidelines is because they not only matter for maintainers, but also for users. As a user I want to know what the library I use considers a breaking change.
All that being said, definitely not something that is actionable in the scope of this PR.
|
|
||
| For 0.x projects (which, again, no longer apply), we’d switch to use `patch` for any **backwards-compatible change** and `minor` for all **breaking changes** (i.e. disregard feature vs bug). | ||
|
|
||
| ## History |
There was a problem hiding this comment.
Nice! I think this section is super helpful. Good call writing that.
2 participants
Changes
Adds docs for thoughts on how to review, how to release. General maintainer guidelines
How to Review
Read it!!! 🙏
Checklist
No, seriously, read it please and give feedback