-
Notifications
You must be signed in to change notification settings - Fork 211
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
Detect package names added/removed from attw.json as changed #944
Conversation
The way that we install things means that this will cause new failures; we use a filtered install so pnpm is not going to know that it needs to install those packages too and so they'll definitely fail tests. A potential way we can try and do this is to update https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/scripts/pnpm-install.sh to detect that case and tack on a few more filters. That or we move |
const versionPath = isLatest ? dirPath : joinPaths(dirPath, `ts${hi}`); | ||
if (lows.length > 1) { | ||
console.log("testing from", low, "to", hi, "in", versionPath); | ||
if (npmChecks !== "only") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a nit, but if this were written as an early return, I think it may be clearer / less diff-y?
if (npmChecks === "only") {
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s stuff after this block, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable now that it will just run ATTW on those packages removed but unchanged. I have not tested it on a specific PR, though.
I tested it locally on DefinitelyTyped/DefinitelyTyped#68558 and it worked. |
For DefinitelyTyped/DefinitelyTyped#68558 (and others in the future)