You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Jul 1, 2024. It is now read-only.
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.
These are just some highlevel comments, since I didn't have a complete look at the code.
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.
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.)
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
cardIDstoundefinedif PRs were specified, and assertcardIDs!if not.