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

DefinitelyTyped repo overhaul - docs/automatisation/formatting #26847

Open
Hotell opened this issue Jun 26, 2018 · 25 comments
Open

DefinitelyTyped repo overhaul - docs/automatisation/formatting #26847

Hotell opened this issue Jun 26, 2018 · 25 comments

Comments

@Hotell
Copy link
Contributor

Hotell commented Jun 26, 2018

This isn't an issue related to any of type definitions rather to improve this repo and make it more consistent and more user friendly for old/new contributors.

Repo updates

  • we really need consistent formatting ( incorporating prettier is viable choice )
  • provide npm scripts for executing prettier + linter fixes so user doesn't have to do it on it's own manually
  • provide githooks ( pre-commit 👉 formatting and lint fixes applied automatically )
  • provide automated changelog generation with every release ( this is a must have as breaking changes can occur even on fix or feat semver bump, and it's not very user friendly to go through commit log what changed in some particular package )
  • be able to run whole test suite from local machine and not wait for CI
  • provide some codemods/scripts for updating all packages dependant on some other core package
    • for example: I update react types with some breaking change from type safety perspective ( bump minimum TS version ) or tweak API type coverage, now I have to manually go through 5k+ packages and update all of them ( which is super annoying especially if you have to resolve merge conflicts )
  • enforcing consistent commit messages

Docs updates

  • consistent pull request/issues naming
    • as can be observed a most used pattern is [package-name]: issue/pr description
  • make clear what a breaking change means
    image
  • IMHO it should be a breaking change only if the library bumps semver as breaking ( if we change some types to be better and expose new type-errors, it should not be bumped == same methodology as TypeScript team is doing )

Summary

If we agree at least at some of these points, I can start work on this immediately 💪

cc @sandersn @andy-ms @mhegazy @DanielRosenwasser

@sandersn
Copy link
Contributor

Tagging @RyanCavanaugh, who is also interested in DefinitelyTyped hygiene.

@sandersn
Copy link
Contributor

Also

be able to run whole test suite from local machine and not wait for CI

This is a documentation problem; we already can do this, I just can't remember how without searching around.

@RyanCavanaugh
Copy link
Member

we really need consistent formatting ( incorporating prettier is viable choice )

Agree!

provide npm scripts for executing prettier + linter fixes so user doesn't have to do it on it's own manually
provide githooks ( pre-commit 👉 formatting and lint fixes applied automatically )

I think this would be great.

provide automated changelog generation with every release ( this is a must have as breaking changes can occur even on fix or feat semver bump, and it's not very user friendly to go through commit log what changed in some particular package )

What is your definition of "release"?

be able to run whole test suite from local machine and not wait for CI

As Nathan said, it is possible, probably just needs better documentation. Be warned that it takes a very, very long time

provide some codemods/scripts for updating all packages dependant on some other core package

I don't know what this means as a general functionality, but am on board in principle.

enforcing consistent commit messages

In general we already have a pretty heavyweight process. I don't want to enforce new rules that people have to follow and we have to enforce unless there's a very clear upside. We can already mine which PRs affect which packages and generate a changelog based on the PR titles (which are much easier to edit than commit messages).

consistent pull request/issues naming
as can be observed a most used pattern is [package-name]: issue/pr description

Again I'm not sure what the upside is here, or what the naming convention would be for changes which affect multiple packages

make clear what a breaking change means

Any non-trivia edit is a potential breaking change. We try to recommend that people pin their types dependencies and upgrade intentionally instead of floating for this reason. We don't vary the major version number anyway, so trying to draw an arbitrary line for what's breaking and what's not doesn't seem to give us much.

@binki
Copy link
Contributor

binki commented Jun 29, 2018

@RyanCavanaugh

make clear what a breaking change means

Any non-trivia edit is a potential breaking change. We try to recommend that people pin their types dependencies and upgrade intentionally instead of floating for this reason. We don't vary the major version number anyway, so trying to draw an arbitrary line for what's breaking and what's not doesn't seem to give us much.

I think changes which would introduce new type errors for people should be considered breaking. When type checks fail, I consider my own build broken. If the changes to the type definitions are not backwards compatible and I’m consuming the definitions using semver, my build may break without any action on my part.

So, why not just always bump the major version for each release (where a release is a publish of the package to npmjs)? Types are very brittle and can easily break projects using them unintentionally or even for minor improvements, so it is likely quite difficult to determine what breaks definitions. Bumping major on every release for definition changes would let people install the typings with npm -D and be all set instead of needing to do something special when installing typings to get the proper versioning in their projects.

@RyanCavanaugh
Copy link
Member

So, why not just always bump the major version for each release

This is just a different way of not having a coherent versioning scheme, with the disadvantage that you lose all mapping from the library version to the typing version. What's the latest typing library for foolib v5? Today it's the highest @types/foolib@5.*, and we can maintain separate version chains in a logical way. In the "semver means all is break" world, it's... a query against a remote database.

@binki
Copy link
Contributor

binki commented Jun 29, 2018

Ah. So why is that line in the README, then? Because of it, I assumed there shouldn't be such a correlation between version numbers in the first place.

@thw0rted
Copy link
Contributor

This issue is the first time I've seen anybody mention not running tests locally. It prompted me to search the front page, and I was surprised not to see it called there explicitly. npm run test works for me... for a very limited definition of "works". Over the past week, it seems like every day something new is broken, either due to changes in dtslint or typescript@next or some combination of stale deps installed in my node_modules. (Latest issue is a boatload of spurious strict-export-declare-modifiers and void-return lint violations.) I'd love to see run test called out explicitly in the contributor docs, but I'd also love to see it behave consistently on my machine from day to day.

I had thought I must be doing something wrong, since I didn't see a lot of people here filing issues about broken tests, but maybe the truth is that most people don't run them, so if something is brittle it doesn't get noticed as quickly as it otherwise might.

@Jessidhia
Copy link
Member

Jessidhia commented Feb 9, 2019

I'd like to bump this thread for the sole reason that I have to remember to disable the prettier plugin almost every time I am working on anything in DefinitelyTyped 🤣

Adding a new lint step right now probably won't work well because CI is close to a breaking point (any given change to @types/react may randomly fail depending on how much CPU time the travis VM gets, for example); but if it's just "adding a command you run" to the top level package.json, well... I could do this now, even.

Adding a canonical config to the root would also be very helpful, even if it's an empty config (i.e. prettier defaults), because otherwise customizations done in the vscode settings take precedence. Personally I have semicolons off and single quotes everywhere, for example.

@Jessidhia
Copy link
Member

Jessidhia commented Feb 9, 2019

I did it on the entire repository just to see what happens.

master...Jessidhia:prettier

Jessidhia/DefinitelyTyped@prettier...Jessidhia:prettier-lets-see-what-happens

25401 files changed, 1959969 insertions(+), 1425876 deletions(-)

@ExE-Boss
Copy link
Contributor

I’m doing this piecemeal, starting with #34894 (and #34892)

@damassi
Copy link
Contributor

damassi commented May 21, 2019

This just happened to me as well -- I ended up running a commit with local prettier enabled and it introduced an unneeded diff. Would be great if there was some consistency here, including pre-commit hooks that apply lint --fix and formatting.

@alloy
Copy link
Collaborator

alloy commented May 22, 2019

As a maintainer, I would love to see a standardised prettier config, githooks enforce formatting, and perhaps prettier checking on CI. I have been running prettier on some of the types I maintain manually with the following configuration, which I sourced from looking at what some randomly picked DT types were doing:

$ prettier --parser typescript --tab-width 4 --semi --trailing-comma es5 --write --print-width 120

@alloy
Copy link
Collaborator

alloy commented May 23, 2019

Another stab at adding prettier and git pre-commit hook is available at #35672

@damassi
Copy link
Contributor

damassi commented May 30, 2019

@RyanCavanaugh - wondering if you have any thoughts on #35672?

@rohit-gohri
Copy link
Contributor

rohit-gohri commented Feb 13, 2020

The command npm run prettier mentioned in Readme(here) doesn't work.

The output is :npm ERR! missing script: prettier

The command should be npx prettier path/to/package/** --write I think?

@alloy
Copy link
Collaborator

alloy commented Feb 13, 2020

@rohit-gohri prettier is listed as a dev dependency, so after doing a npm install it should work.

@rohit-gohri
Copy link
Contributor

@rohit-gohri prettier is listed as a dev dependency, so after doing a npm install it should work.

But there is no corresponding script. AFAIK only yarn runs package bins directly(yarn prettier works), and npm has npx to run local/global packages or temporary install and run them.

@alloy
Copy link
Collaborator

alloy commented Feb 13, 2020

Ah I see, yeah that might have been based on yarn indeed. Care to send a PR to add the script?

@montogeek
Copy link

It is there any plan to create a changelog/website/docs something?

@jadamec
Copy link

jadamec commented Jan 5, 2021

What about Changelog, please?

@orta orta closed this as completed Aug 3, 2021
@orta
Copy link
Collaborator

orta commented Aug 3, 2021

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.

@thw0rted
Copy link
Contributor

thw0rted commented Aug 3, 2021

@orta this is a meta issue, please re-open.

Aside: does this repo not have issue tags? Maybe a "meta" tag isn't needed after the transition because all open Issues should be meta?

@orta
Copy link
Collaborator

orta commented Aug 3, 2021

I'll re-open, but I'm not sure how actionable this issue is nowadays - a lot of the requests have been done and others ideally should have a focused issue like #54239 does.

@orta orta reopened this Aug 3, 2021
@thw0rted
Copy link
Contributor

thw0rted commented Aug 3, 2021

Agreed, this issue really should be at least 3-6 sub-issues but there's a lot of conversation already. I guess anybody watching who has a particular interest in one of the OP's bullet points should find or open an issue about it and link back here, and when they're all accounted for then close this one again?

@jakebailey
Copy link
Member

jakebailey commented Feb 9, 2024

2 years on, I'm even less sure this issue should be open. We have a formatter with instructions for how to use it plus bot comments that say what to do, we have optional git hooks, we can run in CI, CI is now much faster after I sharded it, we have docs that talk about breaking changes.

The only thing from the original thread that is "missing" is some sort of changelog system, some sort of generic codemod system, and "consistent" commit messages.

I don't personally feel like a codemod system is in scope. Changelogs can't be published within the package (as they would grow indefinitely), so don't really have anywhere else to go (but are pretty easy to emulate just by viewing the git history of a specific package dir). We already suggest a specific PR title when modifying packages (though people often ignore it), and rewriting them is not super viable unless PRs only touch one package or something.

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

No branches or pull requests