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

Add download-git-repo #56874

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LinqLover
Copy link
Contributor

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change.
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm test <package to test>.
  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • Represents shape of module/library correctly
  • tslint.json should contain { "extends": "dtslint/dt.json" }, and no additional rules.
  • tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

Requires #56873!

@typescript-bot
Copy link
Contributor

typescript-bot commented Oct 29, 2021

@LinqLover Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

This PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ❌ Continuous integration tests have failed
  • 🕐 Only a DT maintainer can approve changes when there are new packages added

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": 56874,
  "author": "LinqLover",
  "headCommitOid": "3efba07e66b576a9a9c09f33658547a0a94c99ee",
  "mergeBaseOid": "bd82c8c73308f3e343588f37045d5b8c921a4663",
  "lastPushDate": "2021-11-21T10:57:46.000Z",
  "lastActivityDate": "2022-01-15T19:44:51.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "download-git-repo",
      "kind": "add",
      "files": [
        {
          "path": "types/download-git-repo/download-git-repo-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/download-git-repo/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/download-git-repo/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/download-git-repo/tslint.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) (check: `extends`)"
        }
      ],
      "owners": [],
      "addedOwners": [
        "DefinitelyTyped"
      ],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "peterblazejewicz",
      "date": "2021-11-21T10:57:04.000Z",
      "abbrOid": "228bed7"
    }
  ],
  "mainBotCommentID": 955043546,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/3efba07e66b576a9a9c09f33658547a0a94c99ee/checks?check_suite_id=4420462707"
}

@typescript-bot typescript-bot added the New Definition This PR creates a new definition package. label Oct 29, 2021
@typescript-bot
Copy link
Contributor

🔔 @LinqLover — there are no owners, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...)

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Oct 29, 2021
@typescript-bot
Copy link
Contributor

@LinqLover The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@LinqLover
Copy link
Contributor Author

From the CI failure:

Error in download-git-repo
Error: /home/runner/work/DefinitelyTyped/DefinitelyTyped/types/download-git-repo: Contains the word 'download', which is banned by npm.
    at assertPathIsNotBanned (/home/runner/work/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:232:15)
{}
    at /home/runner/work/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:136:13

Please help! Am I supposed to file another PR against https://github.com/microsoft/dtslint/blob/6a0d0a8e626422018afcd920b1167f9c3dea6f86/src/index.ts#L237-L246 and add download-git-repo there?

LinqLover added a commit to LinqLover/dtslint that referenced this pull request Oct 29, 2021
This package already exists on npm, so if I understand the matter correctly, it should be allowed here, too:

https://www.npmjs.com/package/download-git-repo

Complements ad7eb60. Required for DefinitelyTyped/DefinitelyTyped#56874.
peterblazejewicz added a commit to peterblazejewicz/dtslint that referenced this pull request Nov 21, 2021
The assertion is not a rule, so it cannot be bypassed in DT config.

Required for: DefinitelyTyped/DefinitelyTyped#56874

Thanks!

/cc @LinqLover
@peterblazejewicz
Copy link
Member

Hey, sorry for delay, just saw this.
I"ve added a PR: microsoft/dtslint#350
yes, it cannot be corrected by rule, as there is no official rule in linter for 'npm-banned-types', just assertion based on conventions.

'no-consecutive-blank-lines'
@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Nov 21, 2021
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Nov 21, 2021
@typescript-bot
Copy link
Contributor

@LinqLover The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@LinqLover
Copy link
Contributor Author

Thank you very much @peterblazejewicz, is there anything left to do at my end? :)

@peterblazejewicz
Copy link
Member

I think, no, just wait for upstream change, thx!

peterblazejewicz added a commit to peterblazejewicz/dtslint that referenced this pull request Nov 22, 2021
The assertion is not a rule, so it cannot be bypassed in DT config.
- 'download-git-repo'
- 'download-package-tarball'

Both are published to NPM registry already.

Required for:
DefinitelyTyped/DefinitelyTyped#56874
DefinitelyTyped/DefinitelyTyped#56875

Thanks!

/cc @LinqLover
@typescript-bot typescript-bot added the Check Config Changes a module config files label Dec 2, 2021
@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Dec 16, 2021
@typescript-bot
Copy link
Contributor

@LinqLover I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Dec 21st (in a week) if the issues aren't addressed.

@LinqLover
Copy link
Contributor Author

No, this PR should not be canceled. Please refer to the issue in microsoft/dtslint#350 (comment).

@typescript-bot typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Dec 22, 2021
@typescript-bot
Copy link
Contributor

@LinqLover I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Jan 21st (in a week) if the issues aren't addressed.

@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jan 15, 2022
@LinqLover
Copy link
Contributor Author

No, this PR should not be canceled. Please refer to the issue in microsoft/dtslint#350 (comment).

@typescript-bot typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Jan 15, 2022
@peterblazejewicz
Copy link
Member

@LinqLover eventually change to draft

@LinqLover
Copy link
Contributor Author

@peterblazejewicz Doesn't a draft indicate "don't look at me, I'm not yet done"? This PR is ready from my side. We only need to get the infrastructure fixed so that the typing package can actually be published

@peterblazejewicz
Copy link
Member

there is no other way to stop Ci builds to be run from time to time, I understand MS folks will look into how to change how this works now (reading Sander's comment)

@LinqLover
Copy link
Contributor Author

Well, I hope that the PR will not completely do down as a draft PR. There should be a label that prevents the CI builds from commenting again and again.

@LinqLover LinqLover marked this pull request as draft January 15, 2022 20:03
@LinqLover LinqLover mentioned this pull request Feb 23, 2022
12 tasks
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

After #67332, this PR will need to be updated to include an .npmignore. See https://github.com/DefinitelyTyped/DefinitelyTyped#npmignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check Config Changes a module config files New Definition This PR creates a new definition package. The CI failed When GH Actions fails
Projects
Status: Needs Author Action
Development

Successfully merging this pull request may close these issues.

4 participants