WikiEduDashboard icon indicating copy to clipboard operation
WikiEduDashboard copied to clipboard

fix style/optionalBooleanParam cop

Open manbhav234 opened this issue 9 months ago • 5 comments

NOTE: Please review the pull request process before opening your first PR: https://github.com/WikiEducationFoundation/WikiEduDashboard/blob/master/CONTRIBUTING.md#pull-request-process

What this PR does

This PR attempts to make progress on the issue #5297 by removing the Style/optionalBooleanParameters cop from .rubocop.yml and fix the offenses generated by it. This style changes the definition of default boolean parameters inside function definitions from func_name(bool_val = true) to func_name(bool_val: true).

Screenshots

No UI changes made.

Open questions and concerns

I have only commit changes to function definitions. Even though this fixes all the offenses generated by removing this cop, it's better to add such definition to function calls, i.e, func_name(true) to func_name(bool_val: true) so that it is clear for what variable the boolean value is being passed for. However, adding this would require to scan through the codebase and find the exact function calls for those functions which is a little tedious especially for the "email" function as there are many email functions defined in different classes. What are your thoughts on this ?

manbhav234 avatar Feb 24 '25 20:02 manbhav234

The email functions are, for the most part, using the Rails mailer conventions, so I'm not too worried about those ones. Meeting the requirements of that linting rule is good enough for me, I think.

ragesoss avatar Feb 24 '25 23:02 ragesoss

The changes look fine to me. If the build passes, this should be ready to merge.

ragesoss avatar Feb 24 '25 23:02 ragesoss

Looks like this broke a lot of things.

ragesoss avatar Feb 25 '25 17:02 ragesoss

Looks like this broke a lot of things.

Yeah the issue is the functions now expects keyword arguments instead of positional arguments. So just passing func(true) in place of func(param_value: true) causes errors which says '1 argument given (positional) but 0 required'. Shall I then try to fix these errors or something else needs to be done regarding this rule ?

manbhav234 avatar Feb 25 '25 23:02 manbhav234

@manbhav234 fixing the errors will hopefully get this into a usable state.

ragesoss avatar Mar 03 '25 22:03 ragesoss