WikiEducationFoundation / WikiEduDashboard

Wiki Education Foundation's Wikipedia course dashboard system
https://dashboard.wikiedu.org
MIT License
385 stars 600 forks source link

Fix Rubocop RedundantAssignment offenses #5297 #5816

Closed NateWebber closed 3 weeks ago

NateWebber commented 4 weeks ago

What this PR does

This pull request addresses part of issue #5297, namely the rubocop rule Style/RedundantAssignment. When I enabled the rule there were a total of 32 offenses, which are now fixed.

Open questions and concerns

I noticed that Style/RedundantReturn is in the permanent exceptions, and I think that there could be a similar argument made for making this rule an exception as well (and thus this PR unnecessary). Some methods that literally only assign and then return a variable are pretty clearly better off following this rule, whereas others that are more complicated might be more readable before the change. In lib/wikitext.rb see #mediawiki_to_markdown for an example of the former, and #assignments_to_wikilinks for an example of the latter.

I personally think the code is mostly improved by adhering to this rule but I'd appreciate feedback from someone more familiar with this codebase. It was a quick fix so no harm either way.

NateWebber commented 4 weeks ago

The above push was just squashing the commit where I undid some unintentional changes to unrelated files into the main commit

ragesoss commented 4 weeks ago

Thanks @NateWebber! This is a very nice PR, and I appreciate the analysis you included. Browsing through all of the rule violations, I think my preference would be to move this rule to the permanent exceptions list. In some cases, the variable names are doing the work of explaining what kind of thing is being returned, or making a series of transformations more parallel and easy to read. But some of them are better off with the refactors you've done as well. I'll comment on the individual ones I would like to keep as-is.

NateWebber commented 3 weeks ago

Hi, thanks for the feedback! I figured something like this would ultimately end up making the most sense. I've reverted the methods you indicated and added the rule to the permanent exceptions.

ragesoss commented 3 weeks ago

LGTM!

sentry-io[bot] commented 3 weeks ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎