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

[dotenv-flow] Remove superfluous /// <reference types="node" /> #46734

Merged

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Aug 13, 2020

@typescript-bot typescript-bot added this to Needs Author Action in New Pull Request Status Board Aug 13, 2020
@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

master #46734 diff
Batch compilation
Memory usage (MiB) 62.7 36.4 -42.0%
Type count 8898 2138 -76%
Assignability cache size 1613 101 -94%
Language service
Samples taken 36 36 0%
Identifiers in tests 36 36 0%
getCompletionsAtPosition
    Mean duration (ms) 379.5 93.1 -75.5% 🌟
    Mean CV 12.3% 27.9%
    Worst duration (ms) 473.8 118.5 -75.0% 🌟
    Worst identifier encoding parsed
getQuickInfoAtPosition
    Mean duration (ms) 369.7 92.0 -75.1% 🌟
    Mean CV 11.0% 27.0% +146.5%
    Worst duration (ms) 445.6 114.5 -74.3% 🌟
    Worst identifier purge_dotenv encoding

Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Better typescript-bot determined that this PR improves compilation performance. label Aug 13, 2020
@jablko jablko marked this pull request as ready for review August 13, 2020 18:01
@typescript-bot typescript-bot added the Untested Change This PR does not touch tests label Aug 13, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 13, 2020

@jablko Thank you for submitting this PR!

This is a live comment which I will keep updated.

This PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Have you considered adding tests to cover the change you're making? Including tests allows this PR to be merged by yourself and the owners of this module. This can potentially save days of time for you.

Code Reviews

This PR can be merged once it's reviewed by a DT maintainer.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Only a DT maintainer can approve changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 46734,
  "author": "jablko",
  "owners": [
    "vincentlanglet",
    "kerimdzhanov"
  ],
  "dangerLevel": "ScopedAndUntested",
  "headCommitAbbrOid": "ca4565e",
  "headCommitOid": "ca4565e80451e18e1fcca6e3332527aae70c9522",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastPushDate": "2020-08-13T17:52:24.000Z",
  "reopenedDate": "2020-08-13T18:01:24.000Z",
  "lastCommentDate": "2020-08-13T17:52:24.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46734/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "anyPackageIsNew": false,
  "packages": [
    "dotenv-flow"
  ],
  "files": [
    {
      "path": "types/dotenv-flow/index.d.ts",
      "kind": "definition",
      "package": "dotenv-flow"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-08-14T09:38:41.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 6,
  "isChangesRequested": false
}

@typescript-bot typescript-bot moved this from Needs Author Action to Needs Maintainer Review in New Pull Request Status Board Aug 13, 2020
@typescript-bot
Copy link
Contributor

🔔 @VincentLanglet @kerimdzhanov — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌻

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Aug 13, 2020
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Aug 13, 2020
Copy link
Contributor

@kerimdzhanov kerimdzhanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge

@typescript-bot typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Aug 14, 2020
@andrewbranch andrewbranch merged commit fc17c7f into DefinitelyTyped:master Aug 14, 2020
@typescript-bot typescript-bot removed this from Waiting for Author to Merge in New Pull Request Status Board Aug 14, 2020
@typescript-bot
Copy link
Contributor

I just published @types/dotenv-flow@3.0.1 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintainer Approved Owner Approved A listed owner of this package signed off on the pull request. Perf: Better typescript-bot determined that this PR improves compilation performance. Self Merge This PR can now be self-merged by the PR author or an owner Untested Change This PR does not touch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants