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

Don't automatically recommend creating an issue on DefinitelyTyped #331

Merged
merged 1 commit into from Sep 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -8,10 +8,6 @@ patch-package 0.0.0
• Diffing your files with clean files
✔ Created file patches/@microsoft+mezzurite-core++@types+angular+1.6.53.patch

💡 @types/angular is on GitHub! To draft an issue based on your patch run

yarn patch-package @microsoft/mezzurite-core/@types/angular --create-issue

END SNAPSHOT"
`;

Expand Down
28 changes: 28 additions & 0 deletions src/createIssue.test.ts
@@ -0,0 +1,28 @@
import { shouldRecommendIssue } from "./createIssue"

describe(shouldRecommendIssue, () => {
it("Allows most repos", () => {
const eigen = shouldRecommendIssue({
org: "artsy",
repo: "eigen",
provider: "GitHub",
})
expect(eigen).toBeTruthy()

const typescript = shouldRecommendIssue({
org: "Microsoft",
repo: "TypeScript",
provider: "GitHub",
})
expect(typescript).toBeTruthy()
})

it("does not recommend DefinitelyTyped", () => {
const typescript = shouldRecommendIssue({
org: "DefinitelyTyped",
repo: "DefinitelyTyped",
provider: "GitHub",
})
expect(typescript).toBeFalsy()
})
})
16 changes: 15 additions & 1 deletion src/createIssue.ts
Expand Up @@ -46,12 +46,26 @@ function getPackageVCSDetails(packageDetails: PackageDetails) {
}
}

export function shouldRecommendIssue(
vcsDetails: ReturnType<typeof getPackageVCSDetails>,
) {
if (!vcsDetails) {
return true
}

const { repo, org } = vcsDetails
if (repo === "DefinitelyTyped" && org === "DefinitelyTyped") {
Copy link

Choose a reason for hiding this comment

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

I wouldn't like such PR to be merge, blocking any repository. Where do we stop people suggesting blocking their repos?

The problem is solved by the community, and encouraging better collaboration, and discipline. This doesn't solve the problem properly either scales as a fix.

Copy link

Choose a reason for hiding this comment

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

Also, something super clear, I strongly agree with the underline sentiment and intent, but I strongly disagree with the proposed fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean DT is pretty much the biggest public collection of npm packages, I'm sure it's easy to say "well DT has ~7,000 packages to cover"

Copy link

Choose a reason for hiding this comment

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

Sorry Orta, I am not sure what you are trying to coney right now, would you mind expanding on that?

return false
}
return true
}

export function maybePrintIssueCreationPrompt(
packageDetails: PackageDetails,
packageManager: PackageManager,
) {
const vcs = getPackageVCSDetails(packageDetails)
if (vcs) {
if (vcs && shouldRecommendIssue(vcs)) {
console.log(`💡 ${chalk.bold(packageDetails.name)} is on ${
vcs.provider
}! To draft an issue based on your patch run
Expand Down