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.

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

fix style/optionalBooleanParam cop #6211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manbhav234
Copy link

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 ?

@ragesoss
Copy link
Member

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
Copy link
Member

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

@ragesoss
Copy link
Member

Looks like this broke a lot of things.

@manbhav234
Copy link
Author

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants