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

Specify closed PRs to run.ts #354

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Jan 27, 2021

Just a thought, it might be nice to be able to specify closed PRs to run.ts? Currently you can specify a draft PR and run.ts will trigger the bot on it, however if you specify a closed PR run.ts silently terminates (shouldRunOn() is always false).

This PR skips the all-open-prs query if PRs were specified and triggers the bot on them, open or otherwise.

Like in #352 I added the PR number to the projectboard-cards query, dropped the card ID from the all-open-prs query, and exclude all-open-prs from projectboard-cards on PR number vs. card ID, however in this case it's just to make the code a little tidier. Otherwise I'd need to initialize cardIDs to undefined if PRs were specified, and assert cardIDs! if not.

Comment on lines +8 to +10
export function flatten<T>(xs: T[]) {
type U = T extends (infer U)[] ? U : T;
return ([] as U[]).concat(...xs as U[]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Types for when arguments aren't arrays, e.g. flatten([["a"], "b"]) -> ["a", "b"]

@elibarzilay
Copy link
Contributor

These are just some highlevel comments, since I didn't have a complete look at the code.

  1. I'm not sure what's the goal here. There is an implicit assumption that the bot does nothing with closed PRs, other than potentially removing them from the board as part of the cleanup, and this changes this behavior to something else (which, again, I'm not clear about, and I don't know if there's an overall different design that you see).

    I did consider forcing running on closed PRs, but only for the purpose of creating post-mortem test fixtures, which I do by changing the code to fake its state (so the usual information is still collected). So what I was thinking was some organized way of doing it, including the fake state, but it looks like this is not only different, but it would prohibit such a future extension.

  2. In general, I'm trying to somewhat minimize the maintenance time for the bot, so it would help if you have smaller PRs... I've seen some big-ish query-related changes in some of the recent PRs, and since they can be tricky (and dangerous in breaking functionality) it'll take me some time to get to go over them. It would also help to have some clear goals so we know that there's a purpose for changes.

    (To be clear, I'm all for improving code, and I certainly had a good amount of that when I took over the bot. But there's always the consideration that this is a live and useful thing so improvements should be weighted against the possibility of breakage. It sometimes pains me, but there are cases where "if it ain't broken" is too strong to ignore.)

  3. Sidenote: I'm not sure about the flatten change: I generally dislike the implicit T[] | T of .concat, which for some reason got dragged into .flat too... It would be nice if there's a way to do the same without the type change.

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

Successfully merging this pull request may close these issues.

None yet

2 participants