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.

Sign up for GitHub

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

Update issue manager with "feedback requested" behavior #361

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Update issue manager with "feedback requested" behavior #361

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eeed2f2
Update issue manager with "feedback requested" behavior
mfisher87 Nov 22, 2023
5b8a818
Pin issue-manager action dependency
mfisher87 Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions .github/workflows/issue-manager.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,28 @@ name: "Issue Manager"

on:
schedule:
- cron: "0 0 * * *"
- cron: "0 0 * * *"
issue_comment:
types:
- "created"
issues:
types:
- "labeled"
pull_request_target:
types:
- "labeled"

jobs:
issue-manager:
runs-on: ubuntu-latest
runs-on: "ubuntu-latest"
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 feel strongly that quoting YAML strings is important. There's too many weird things that can happen if you don't. Just like bash :) Is there a pre-commit validator that can check for this? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

GitHub's own documentation doesn't quote simple YAML strings, except for ${{}} contexts, e.g.,
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idruns-on

Other than a few special cases (number-y strings and special characters) I've not had any issues with unquoted strings in YAML. So, I feel it's entirely unnecessary and would prefer to match GitHub's documentation examples as much as possible.

Copy link
Collaborator Author

@mfisher87 mfisher87 Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is something with an objectively correct answer, but I also don't really consider GitHub an authority on YAML quality, so I'm not terribly swayed in my opinion by that :) One of the strange thing YAML does is convert the value no (the word "no" or the country code for Norway) to False. String rules in YAML are very complicated. I think "explicit is better than implicit", and "simple is better than complex" are my guiding principles in this preference.

If you feel strongly against adding the quotes, I don't think we're going to resolve that in the context of this PR :) I'm happy to restore this to a mix of quoted and unquoted stings as it was before (because I don't think we should enforce any standard when we don't have consensus) so we can get this merged!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Following up on the earlier thought about validation, there is a yamllint rule that can enforce always-quoted strings (single or double), never enforce quotes, or permit quotes only when needed (for all the special cases like no, yes, true, false, #, [], {}, \d.*... probably more I'm not aware of). If/when we have consensus on this issue, I think it'd be great to record that decision as a linting rule.

https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.quoted_strings

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think my preference is strong enough to warrant a standoff 🤣

I've just rarely hit this issue in practice, and almost always with trailing zeros on number-y things. So I see a molehill here, but if you've climbed a mountain and planted a flag, that's cool with me -- we'll go with that.

I do like the idea of making a decision and letting the linter-bot be the sheriff, though!

Copy link
Collaborator Author

@mfisher87 mfisher87 Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I do like the idea of making a decision and letting the linter-bot be the sheriff, though!

Yes! Having software solve these problems is much better than dealing with competing preferences. There are lots of usually-not-a-big-deal things people can have strong opinions about, and for me quoting strings in YAML and Bash is one of them. I've been bitten too many times, and "just quote strings" keeps my cognitive load low :) But I don't want things that help me to cause harm to anyone else, so... yeah, let's make this a discussion about linters/formatters :) Opened #364

steps:
- uses: tiangolo/issue-manager@master
- uses: "tiangolo/issue-manager@0.4.0"
with:
token: ${{ secrets.GITHUB_TOKEN }}
token: "${{ secrets.GITHUB_TOKEN }}"
config: >
{
"answered": {
"users": ["betolink"],
"feedback requested": {
"delay": 864000,
"message": "Assuming the original issue was solved, it will be automatically closed now. But feel free to add more comments or create new issues."
"message": "Closing after 10 days of waiting for feedback. If you feel this was in error, please re-open, `@` a maintainer, or create new issues."
}
}
Loading