Skip to content

rescue both standarderror and exceptions here - #5827

Merged
johrstrom merged 4 commits into
masterfrom
johrstrom-patch-1
Sep 23, 2026
Merged

johrstrom merged 4 commits into
masterfrom
johrstrom-patch-1

Conversation

@johrstrom

@johrstrom johrstrom commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Please review our Contributing Guide before submitting a pull request.

What does this PR do, and what is the related issue, if applicable?

rescue both standarderror and exceptions here because currently we only rescue standarderrors when exceptions can happen with bad ruby code in ERB files.

Testing

  • Tests included
  • If a maintainers' assistance is needed for tests, add label test help
  • No test is needed because _____ (documentation fix, dependency update, etc.)

Documentation

NA

Code Authorship & Understanding

  • I can explain what this code does and answer questions about it
  • This PR was generated or assisted by AI tools (Copilot, Claude, agents). If so, I have reviewed and tested the code
    and I take responsibility for its correctness.

Checklist

  • Follows project code style and conventions
  • This is a large pull request and was discussed first in an issue or with the maintainers.

Anything else?

NA

rescue both standarderror and exceptions here because currently we only rescue standarderrors when exceptions can happen with bad ruby code in ERB files.
yml = YAML.safe_load(content, aliases: true) || {}
conf.deep_merge!(yml.deep_symbolize_keys)
rescue => e
rescue Exception, StandardError => e

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At https://docs.ruby-lang.org/en/master/Exception.html it states that StandardError is a subclass of exception, so rescuing both may be extraneous. I could be convinced if we think it is clearer, but otherwise just rescuing Exception should be ok

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, It's just Exception now.

@Bubballoo3 Bubballoo3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@johrstrom
johrstrom merged commit a91b697 into master Sep 23, 2026
49 of 50 checks passed
@johrstrom
johrstrom deleted the johrstrom-patch-1 branch September 23, 2026 14:05
@github-project-automation github-project-automation Bot moved this from Awaiting Review to Merged/Closed in PR Review Pipeline Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Merged/Closed

Development

Successfully merging this pull request may close these issues.

3 participants