Adds some admin docs #41063
Adds some admin docs #41063
Conversation
|
@orta Thank you for submitting this PR! Because this PR doesn't have any code reviewers, a DefinitelyTyped maintainer will be reviewing it in the next few days once the Travis CI build passes. In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day! |
typescript-bot
added
the
Unowned
| ##### New DefinitelyTyped Package | ||
|
|
||
| - Is the author also a maintainer of the library? If so, they could use [`--declaration` and `--allowJs`](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#--declaration-and---allowjs) in TypeScript to generate their types. | ||
|
|
There was a problem hiding this comment.
at some point we should have the content of 'oh no modules' in here, plus links to the handbook. for now can you add a list like
- no exceptions in tslint.json
strict: trueor equivalent four (five?) settings in tsconfig.json- no
esModuleInterop: true, no matter how tempting - make sure export default is actually default in the source; react-* packages get a pass on this.
Also maybe link to my slide deck: https://docs.google.com/presentation/d/1Q4xfZSY7d9yHhtxSyb-DE85fTXB38RyF3nnyVyvenwc/edit?usp=sharing
|
|
||
| Welcome! This is a resource for the person who is on call for DefinitelyTyped. The TypeScript team always has someone | ||
| assigned to DefinitelyTyped duty, where they do a week on-call. You can see the [schedule here](http://aka.ms/DTRotation). | ||
|
|
There was a problem hiding this comment.
These two paragraphs are a good start, although eventually I'd like to have a bit more about the philosophy — that this isn't a code review in the usual sense, but an assessment of category and severity, followed by delegating to the people who actually know how to review the code. Since the bot tags the PRs with category and owners, it's mainly an exercise in co-ordinating between humans, and filling in any places the bot currently misses.
Also, something about how the sliding scale of package popularity affects reviews. The maintainer should review changes to popular packages, and very popular packages — those with a name you recognise, roughly — should have signoffs from at least two maintainers. Conversely, it's fine to overlook problems with changes to unpopular packages; in many cases only the author is using them anyway.
There was a problem hiding this comment.
I agree - I've expanded it!
typescript-bot
moved this from Review
to Needs Author Attention
in Pull Request Status Board
typescript-bot
added
the
Revision needed
|
@orta One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
|
thanks bot |
|
Gave it another one-over 👍 |
docs/admin.md
Outdated
| Constraints which you should consider: | ||
|
|
||
| - Is it popular? (e.g. do you recognise it) if so, it should probably have two sign-offs | ||
| - Will the PR break existing code? If so, check that there's a semver bump |
There was a problem hiding this comment.
This is hard in two ways:
- Anything that's not a strict addition is likely to break code; and some additions break code, for example if they're to a class with subclasses.
- DT packages are supposed to match the source's major.minor, so to bump semver, breaking changes can only ship when updating the types to a new major version.
The result is that so far we've told people that the Right Way to consume DT packages is to pin them to an exact major.minor.patch. That's not in the README and it's not common practise, but I think those are the things to change.
|
🔔 @sandersn - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
Co-Authored-By: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
|
@orta The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
typescript-bot
added
The Travis CI build failed
and removed
The Travis CI build failed
labels
|
@orta The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@orta I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
|
Alright, I'm not gonna fight the bot - this is good for now 👍 |
This PR adds some docs for admins with an overview of some useful repos and does some minor cleanup on the README