Fix Violations of and Reenable Style/RescueModifier

[Hamms]

The official Ruby Style guide recommends avoiding the use of rescue in its modifier form. It does somewhat beg the question of why include a modifier form in the first place, but that's just by the by.

Quoting from Rubocop documentation, this rule:

Checks for uses of rescue in its modifier form is added for following reasons:

• The syntax of modifier form rescue can be misleading because it might lead us to believe that rescue handles the given exception but it actually rescue all exceptions to return the given rescue block. In this case, value returned by handle_error or SomeException.
• Modifier form rescue would rescue all the exceptions. It would silently skip all exception or errors and handle the error. Example: If NoMethodError is raised, modifier form rescue would handle the exception.

Initial fix applied automatically with bundle exec rubocop --autocorrect-all --only Style/RescueModifier; I then manually ran a few more autocorrect commands to fix up some of the indentation and layout errors produced by the initial autocorrection.

In general, I think Style/ Cops are always optional, so I'm quite open to pushback on enabling this rule if folks want to continue to be able to use rescue in its modifier form. I lean towards being in favor of enabling it, myself; although I appreciate the aesthetics of inlining a rescue clause, I put a lot of weight on the official Ruby style guide's recommendations, and the reasons laid out in Rubocop's documentation seem compelling.

What do y'all think?

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Links

• https://rubystyle.guide/#no-rescue-modifiers
• https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/RescueModifier
• https://docs.rubocop.org/rubocop/cops_style.html#stylerescuemodifier

Testing story

Relying on existing tests to verify that this does not result in any change to functionality.

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[davidsbailey]

While I do like the original syntax because it allows you to write more concisely, the somewhat unexpected behavior of the syntax does make it a bit of a land mine if you are new to it and do not take the time to look up what it does (see inline comment).

since the obscurity of the syntax makes it harder to read and somewhat error prone, I'm inclined to vote yes on this PR. however I am also interested to hear what others think :-)

[davidsbailey]

I have to say, this looks like a potential misuse of the "modifier" syntax -- it looks like the caller wanted to rescue this type of error, rather than insert this error class into the list of returned ids. in fact it looks like this error was introduced specifically when switching to the modifier syntax: d04d412

[Hamms]

Oh, great catch! Definitely agree with that analysis; I'm tempted to manually update this fix to reflect that presumed intent rather than the current functionality.