Plots2: Ruby Style Guide [Rubocop]

[siaw23]

Plots2 has a .rubocop_todo.yml. Is someone willing to take up the following to fix them? If you need help with something please ask and we'll be happy to help.

For starters the following can be fixed first.

Layout/AccessModifierIndentation
Layout/AlignArray
Layout/AlignHash

Once you're done with your changes, please run rake test and rubocop. Create a PR when all passes.

[namangupta01]

i got 140 offences after running rubocop
i'll fix them all

[namangupta01]

Results: https://pastebin.com/raw/J2kxYX66

EDIT: Moved results to pastebin for keeping the conversation short and simple.

[siaw23]

awesome, go for it! :)

[namangupta01]

i am getting offences in public/lib but i'am not sure if we should correct it or not ? or should i simply add this folder in .rubocop.yml

[namangupta01]

In line : https://github.com/publiclab/plots2/blob/6b4726820e26cecf1acf379b2b62412928083bfa/app/services/typeahead_service.rb#L91
What does srchString == 0 suppose to do here?

[siaw23]

unless srchString.nil? || srchString == 0 that line simply means if srchString is not nil (in other words if there's actually a srchString) OR if srchString is not the figure 0, do the following.

but since you pointed this out i think someone meant to compare length instead, if that's the case, .nil? would be enough. what do you think @jywarren

[namangupta01]

so we are comparing string with a integer here? And i have open a pr for this issue can you please have a look at that. And i have also included public/lib files to exclude from rubocop offences because i was thinking it is not good to edit public/lib files what do you say?

[siaw23]

for now i have left public/lib out until we fix all the offences, we'll enable the rest one by one while we progress with this. please show me your PR relating to this @namangupta01

[namangupta01]

I don't get why we are comparing with 0, i mean what is the need of this. Can any one please tell me? @siaw23 @ryzokuken @jywarren @publiclab/reviewers

[namangupta01]

@siaw23 pr is #2127

[ryzokuken]

I agree with @siaw23, I guess they wanted to check if its length is equal to zero and mistyped. If they wrote tests for each border case, then looking into those will give you a good idea of what they really intended.

[ryzokuken]

P.S. @david-days wrote this part 2 years ago.

[namangupta01]

@ryzokuken i didn't find any tests. Can you please tell me where can i find it?

[ryzokuken]

That part must be covered somewhere. Maybe try using CodeCov to find out the tests written against it?

[seafr]

srchString == 0 is probably meant to check if srchString is blank as it is a url param (see test/functional/search_api_test.rb and test/functional/typeahead_api_test.rb)

[ryzokuken]

Good catch! That's probably it. @namangupta01 what was the original problem though? What was wrong with that particular line of code?

[namangupta01]

@ryzokuken It's an rubocop offence

[ryzokuken]

Indeed it is! That's because there's a method for this which happens to be much more readable. Switch to that method instead, please.

[namangupta01]

Including that method is causing test failure . See pr #2127

[ryzokuken]

Try using your_string_variable.empty? instead.

[namangupta01]

Oh! thank you @ryzokuken I will update it!

[ryzokuken]

@seafr if the param isn't passed at all, it is nil. If it is, but there's no value, it's probably empty. So I think @david-days wanted to check if it's length was zero and used this accidentally, thus trying to compare a string variable to a numeric literal.

[seafr]

@ryzokuken yes, this is what I think, else comparing a string to an integer - as @namangupta01 mentioned above - doesn't make sense.
As for the .empty? method, what do you think of using Rails' .blank? method, just in case a string of white spaces is passed?

[ryzokuken]

Hmm, that does make sense. Searching a blank (whitespace-only) string doesn't make sense.

@namangupta01 while this specific example will work better with .blank?, please use the best possible method for each one of the cases, I will look into them while reviewing your PR.