Adding rubocop sample config #547

alaxalves · 2019-04-17T12:49:50Z

Fixes #543 (comment) (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

codeclimate · 2019-04-17T12:50:35Z

There are too many results to compare

View more on Code Climate.

kaustubh-nair · 2019-04-18T18:29:45Z

Some important options which I think we need to add:

Increase line size
Do not enforce single quotes
I guess we can encourage contributors to run rubocop locally before creating a PR?
This will ensure there are lesser issues.

…ubocop-config

alaxalves · 2019-04-19T13:29:21Z

Some important options which I think we need to add:

Increase line size

Do not enforce single quotes
I guess we can encourage users to run rubocop locally before creating a PR?
This will ensure there are lesser issues.

Done in https://github.com/publiclab/mapknitter/pull/547/files#diff-11a0d7906801d9dea0eccb85667ad811R125 and I have removed the line which enforces double quotes.

codecov · 2019-04-19T13:32:23Z

Codecov Report

Merging #547 into main will decrease coverage by 1.1%.
The diff coverage is 69.9%.

@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
- Coverage   71.27%   70.17%   -1.11%     
==========================================
  Files          33       34       +1     
  Lines        1295     1321      +26     
==========================================
+ Hits          923      927       +4     
- Misses        372      394      +22

Impacted Files	Coverage Δ
app/controllers/users_controller.rb	`100% <ø> (ø)`	⬆️
app/controllers/annotations_controller.rb	`33.33% <0%> (ø)`	⬆️
app/controllers/application_controller.rb	`83.87% <0%> (-5.79%)`	⬇️
app/mailers/comment_mailer.rb	`100% <100%> (ø)`	⬆️
app/models/export.rb	`94.44% <100%> (ø)`	⬆️
app/controllers/maps_controller.rb	`90.51% <100%> (ø)`	⬆️
lib/exporter.rb	`92.07% <100%> (ø)`	⬆️
app/models/comment.rb	`100% <100%> (ø)`	⬆️
app/controllers/feeds_controller.rb	`100% <100%> (ø)`	⬆️
app/controllers/tags_controller.rb	`90.9% <100%> (ø)`	⬆️
... and 12 more

alaxalves · 2019-05-15T01:13:56Z

So is this still wanted @jywarren @SidharthBansal @sashadev-sky @icarito @cesswairimu ?

sashadev-sky · 2019-05-20T09:35:47Z

@alaxalves I looked through the configs. So just to confirm, the files listed under "Exclude" are actually the ones that will be copped because they are excluded from being disabled?

One thing I think is good practice is to use the same rubocop rules across repos whenever possible. Should we instead be copying over the config files from plots2, and customizing from there if necessary?

Then also anything that is decided to be too strict we could also remove from plots2.

alaxalves · 2019-05-20T13:55:25Z

@alaxalves I looked through the configs. So just to confirm, the files listed under "Exclude" are actually the ones that will be copped because they are excluded from being disabled?

One thing I think is good practice is to use the same rubocop rules across repos whenever possible. Should we instead be copying over the config files from plots2, and customizing from there if necessary?

Then also anything that is decided to be too strict we could also remove from plots2.

@sashadev-sky The files in exclude are not compiled by Rubocop thus those files are not "judged". It's a good option having the same config provided in Plots2, so I'll use the configs set there.

sashadev-sky · 2019-05-20T21:35:40Z

@alaxalves gotchya! Ok ping me again when its updated

alaxalves · 2019-05-21T00:35:58Z

@alaxalves gotchya! Ok ping me again when its updated

It's done. BTW it was a great idea using the same rules as in Plots2. Thnx for the tip @sashadev-sky

kaustubh-nair · 2019-05-21T07:13:54Z

The shopify style guide is quite huge. Did you test this and see how many offenses it generates on the current codebase?
Also let's get this merged as quickly as possible. All my PRs fail codeclimate checks 😢 😢

jywarren · 2019-05-22T00:19:53Z

Just ping me when this is ready for a merge, thanks everyone!!! 🙌

alaxalves · 2019-05-22T01:27:30Z

The shopify style guide is quite huge. Did you test this and see how many offenses it generates on the current codebase?
Also let's get this merged as quickly as possible. All my PRs fail codeclimate checks

@jywarren @kaustubh-nair @sashadev-sky I'm currently working on those offenses, but there are a lot rn (~30). By this friday I'll get it done.

kaustubh-nair · 2019-05-22T06:20:46Z

@jywarren @kaustubh-nair @sashadev-sky I'm currently working on those offenses, but there are a lot rn (~30). By this friday I'll get it done.

@alaxalves I didn't really go through the shopify style guide properly, but if it is too strict we should not be enforcing it.
I think the enforcing should be as lax as possible to so as not to discourage newcomers.
Thanks

sashadev-sky · 2019-05-23T03:14:56Z

@kaustubh-nair @alaxalves Ok how about we start with the ones that are failing here - 22 issues to fix we can decide based on these what should be fixed and what we should stop copping for because its too strict. -- update just saw @alaxalves is on this so ty 👍

The problem is can you guys see which mistakes specifically are being caught? For me it says cannot generate report because of high volume.

Then @kaustubh-nair we can just move this over to one of your PRs if you're having too many unreasonable codeclimate issues still and decide again what is too strict and remove it.. just deal with it as it comes up

I think before there were 80+ issues to fix on this PR so this file can't be that unreasonable right? It has worked for plots2 for a while

IgorWilbert · 2019-05-24T00:49:17Z

app/models/map.rb

  end

  def self.authors(limit = 50)
    Map.limit(limit)
       .order("maps.id DESC")
       .where('password = "" AND archived = "false"')
       .collect(&:author)
-#       .group("maps.author")
+    #       .group("maps.author")


Shouldn't we delete this commented line?

IgorWilbert · 2019-05-24T01:08:33Z

app/controllers/export_controller.rb

-    else
-      output = 'export has not been run'
-    end
+    output = if export.present?


Does something like this work?

IgorWilbert · 2019-05-24T01:14:56Z

app/controllers/sessions_controller.rb

      if result.successful?
        @user = User.find_by_identity_url(identity_url)
-        if not @user
+        unless @user


If you wanted, I think you could use the not-not here and maybe other places (it is good practice as far as I know), like this:
!!@user
The first ! is to turn nill into true and the second ! is to turn true into false.

IgorWilbert · 2019-05-24T01:21:49Z

app/models/map.rb

-      total_sum = (scales.inject {|sum, n| sum + n }) if scales
-      average = total_sum/count if total_sum
+      total_sum = (scales.inject { |sum, n| sum + n }) if scales
+      average = total_sum / count if total_sum


I don't know if it is the case here, but I guess it is worth mentioning. I was taking much longer reviewing PRs at work because of different settings for formatting on save in the code editors, so people kept doing and undoing space changes. If that is the case, it would be great to define a standard, or format code with some other tool :)

Rules such as Layout/SpaceAroundBlockParameters, Layout/SpaceAroundEqualsInParameterDefault, Layout/SpaceAroundOperators and more try to define such spacing rules. The thing is that after we merge this we should quickly integrate this with our pipeline to encourage contributors to use our code pattern. Otherwise problems such as you're described will keep happening.

sashadev-sky · 2019-05-26T22:28:14Z

@alaxalves what's the progress here? anything I can do to help?

alaxalves · 2019-05-27T12:29:45Z

@alaxalves what's the progress here? anything I can do to help?

Hi @sashadev-sky. I'm currently trying to adjust Mapknitter's code into our rubocop rules(took from Plots2). There are around <15 failling offenses.

sashadev-sky · 2019-05-27T16:48:34Z

@alaxalves what's the progress here? anything I can do to help?

Hi @sashadev-sky. I'm currently trying to adjust Mapknitter's code into our rubocop rules(took from Plots2). There are around <15 failling offenses.

Can you not see what specifically the offences are? It won't generate a report for me

alaxalves · 2019-05-27T17:50:56Z

@alaxalves what's the progress here? anything I can do to help?

Hi @sashadev-sky. I'm currently trying to adjust Mapknitter's code into our rubocop rules(took from Plots2). There are around <15 failling offenses.

Can you not see what specifically the offences are? It won't generate a report for me
@sashadev-sky

sashadev-sky · 2019-05-27T19:41:13Z

I looked over two of them - hope this helps!

URI.decode method is obsolete: fixed this recently in plots2 with @siaw23 you can reference what it was changed to here: https://github.com/publiclab/plots2/blob/c1d3420fb6b1359790230c153dc218530ac448cb/app/controllers/application_controller.rb#L107-L117
replace class var with instance var - tried changing these to instance variables and the code broke. Not fully sure how the class var is holding it up maybe @jywarren remembers?

I think you might also need to update your main branch and rebase again because I saw a few files under "files changed" that were updated from past merged PRs

…ubocop-config

Co-Authored-By: Sasha Boginsky <41092741+sashadev-sky@users.noreply.github.com>

alaxalves · 2019-05-30T14:57:25Z

@sashadev-sky Good news is that I have finished fixing these offenses, I think it's ready to merge.

BTW if this gets merged before #605 I'd like to integrate rubocop checks into Travis pipelines too. If this gets merged after #605 I'll open another PR for Rubocop x Travis integration.

PS: CodeClimate is set waaay different from Rubocop. I think we could keep only Rubocop, since CC lastest version doesn't support Rubocop linting version > 0.52.0, along with the performance cop. And it produces 'fake' errors such as:

.rubocop.yml: Layout/EndAlignment has the wrong namespace - should be Lint
.rubocop.yml: Layout/DefEndAlignment has the wrong namespace - should be Lint
/usr/local/lib/ruby/site_ruby/2.4.0/rubygems/core_ext/kernel_require.rb:122:in `require': cannot load such file -- rubocop-performance (LoadError)
	from /usr/local/lib/ruby/site_ruby/2.4.0/rubygems/core_ext/kernel_require.rb:122:in `require'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:15:in `block in resolve_requires'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:11:in `each'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:11:in `resolve_requires'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader.rb:38:in `load_file'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:87:in `block in base_configs'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:86:in `map'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:86:in `base_configs'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader_resolver.rb:21:in `resolve_inheritance'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader.rb:44:in `load_file'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_loader.rb:79:in `configuration_from_file'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/config_store.rb:44:in `for'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/target_finder.rb:161:in `excluded_dirs'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/target_finder.rb:139:in `find_files'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/target_finder.rb:112:in `target_files_in_dir'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/target_finder.rb:89:in `block in find'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/target_finder.rb:87:in `each'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/target_finder.rb:87:in `find'
	from /usr/local/bundle/gems/rubocop-0.52.1/lib/rubocop/runner.rb:58:in `find_target_files'
	from /usr/src/app/lib/cc/engine/file_list_resolver.rb:15:in `block in expanded_list'
	from /usr/src/app/lib/cc/engine/file_list_resolver.rb:13:in `each'
	from /usr/src/app/lib/cc/engine/file_list_resolver.rb:13:in `flat_map'
	from /usr/src/app/lib/cc/engine/file_list_resolver.rb:13:in `expanded_list'
	from /usr/src/app/lib/cc/engine/rubocop.rb:49:in `files_to_inspect'
	from /usr/src/app/lib/cc/engine/rubocop.rb:29:in `block in run'
	from /usr/src/app/lib/cc/engine/rubocop.rb:28:in `chdir'
	from /usr/src/app/lib/cc/engine/rubocop.rb:28:in `run'
	from /usr/src/app/bin/codeclimate-rubocop:17:in `<main>'

kaustubh-nair · 2019-05-31T15:41:34Z

@alaxalves So what do you think should be done about codeclimate?
We could try using rubocop 0.52 is we have to keep it synced with CC

alaxalves · 2019-06-01T00:47:51Z

@alaxalves So what do you think should be done about codeclimate?
We could try using rubocop 0.52 is we have to keep it synced with CC

That's a good option. I'll test it out by locking rubocop on this version. Thnx.
I have checked in codeclimate's repo and there's a PR opened regarding this already at qltysh/qlty#915. But meanwhile this does not get merged I'll do what you have suggested.

alaxalves · 2019-06-01T01:37:55Z

@alaxalves So what do you think should be done about codeclimate?
We could try using rubocop 0.52 is we have to keep it synced with CC

That's a good option. I'll test it out by locking rubocop on this version. Thnx.
I have checked in codeclimate's repo and there's a PR opened regarding this already at codeclimate/codeclimate#915. But meanwhile this does not get merged I'll do what you have suggested.

@kaustubh-nair Just did that but I'm not being able to see the CC offenses.

jywarren · 2019-06-01T15:31:26Z

Great! Do we need to make a note to not accept dependabot PRs for rubocop then?

jywarren · 2019-06-01T15:31:34Z

Thank you!!!!!!

alaxalves · 2019-06-02T01:19:57Z

Great! Do we need to make a note to not accept dependabot PRs for rubocop then?

Yes, otherwise CodeClimate will start breaking. Thx @jywarren

* Add annotation tests and remove redundant code * Add export tests * Remove some code * Fix codeclimate issues * Fix codeclimate issues * Fix rubocop block length * Removing rubocop cfg since #547 and renaming tests

* Adding rubocop sample config * Removing double_quotes enforcement * Using same rubocop yaml as in Plots2 * Autofixing rubocop offenses * Fixing conditions for CC Co-Authored-By: Sasha Boginsky <41092741+sashadev-sky@users.noreply.github.com> * Adding Performance cop and fixing some offenses * Fixing rubocop offenses and warnings * Downgrading rubocop version since publiclab#547 (comment)

* Add annotation tests and remove redundant code * Add export tests * Remove some code * Fix codeclimate issues * Fix codeclimate issues * Fix rubocop block length * Removing rubocop cfg since publiclab#547 and renaming tests

Adding rubocop sample config

dbae389

alaxalves mentioned this pull request Apr 17, 2019

Attempt to install codecov #543

Merged

alaxalves added 2 commits April 19, 2019 10:24

Removing double_quotes enforcement

4e2fc76

Merge branch 'main' of https://github.com/publiclab/mapknitter into r…

d211cd5

…ubocop-config

alaxalves force-pushed the rubocop-config branch from b0549f9 to d211cd5 Compare April 19, 2019 13:27

alaxalves mentioned this pull request Apr 19, 2019

Integrate Rubocop with Travis #548

Closed

Using same rubocop yaml as in Plots2

5d64101

alaxalves force-pushed the rubocop-config branch from 769efa6 to 5d64101 Compare May 21, 2019 00:39

alaxalves mentioned this pull request May 23, 2019

Removing problematic Warpable test #628

Closed

5 tasks

Autofixing rubocop offenses

0a1fe81

IgorWilbert reviewed May 24, 2019

View reviewed changes

alaxalves and others added 3 commits May 30, 2019 10:35

Merge branch 'main' of https://github.com/publiclab/mapknitter into r…

8c4e724

…ubocop-config

Fixing conditions for CC

33fa04e

Co-Authored-By: Sasha Boginsky <41092741+sashadev-sky@users.noreply.github.com>

Adding Performance cop and fixing some offenses

c14bbf6

alaxalves force-pushed the rubocop-config branch from 1f69925 to c14bbf6 Compare May 30, 2019 13:47

Fixing rubocop offenses and warnings

b0b1e65

This was referenced May 31, 2019

Improving test env and travis workflow #605

Merged

Add tests for controllers #615

Merged

Downgrading rubocop version since publiclab#547 (comment)

c279c4b

alaxalves added a commit that referenced this pull request Jun 1, 2019

Removing rubocop cfg since #547 and renaming tests

0c69df6

jywarren merged commit 713e387 into publiclab:main Jun 1, 2019

alaxalves deleted the rubocop-config branch June 2, 2019 01:19

alaxalves mentioned this pull request Jun 3, 2019

Bump rubocop from 0.52.1 to 0.71.0 #660

Closed

This was referenced Jun 5, 2019

Fix Rubocop performance deprecation warning publiclab/plots2#5795

Closed

Weekly Community Check-In #24 (Learning from Public Lab) publiclab/plots2#5828

Closed

sashadev-sky mentioned this pull request Jun 7, 2019

Call for Reviewers 🎉 #634

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding rubocop sample config #547

Adding rubocop sample config #547

alaxalves commented Apr 17, 2019 •

edited

Loading

codeclimate bot commented Apr 17, 2019

kaustubh-nair commented Apr 18, 2019 •

edited

Loading

alaxalves commented Apr 19, 2019

codecov bot commented Apr 19, 2019 •

edited

Loading

alaxalves commented May 15, 2019 •

edited

Loading

sashadev-sky commented May 20, 2019

alaxalves commented May 20, 2019 •

edited

Loading

sashadev-sky commented May 20, 2019

alaxalves commented May 21, 2019 •

edited

Loading

kaustubh-nair commented May 21, 2019

jywarren commented May 22, 2019

alaxalves commented May 22, 2019

kaustubh-nair commented May 22, 2019

sashadev-sky commented May 23, 2019 •

edited

Loading

IgorWilbert May 24, 2019 •

edited

Loading

alaxalves May 27, 2019

IgorWilbert May 24, 2019

IgorWilbert May 24, 2019

IgorWilbert May 24, 2019

alaxalves May 24, 2019

sashadev-sky commented May 26, 2019

alaxalves commented May 27, 2019

sashadev-sky commented May 27, 2019

alaxalves commented May 27, 2019 •

edited

Loading

sashadev-sky commented May 27, 2019

alaxalves commented May 30, 2019 •

edited

Loading

kaustubh-nair commented May 31, 2019

alaxalves commented Jun 1, 2019 •

edited

Loading

alaxalves commented Jun 1, 2019

jywarren commented Jun 1, 2019

jywarren commented Jun 1, 2019

alaxalves commented Jun 2, 2019

Adding rubocop sample config #547

Adding rubocop sample config #547

Conversation

alaxalves commented Apr 17, 2019 • edited Loading

codeclimate bot commented Apr 17, 2019

kaustubh-nair commented Apr 18, 2019 • edited Loading

alaxalves commented Apr 19, 2019

codecov bot commented Apr 19, 2019 • edited Loading

Codecov Report

alaxalves commented May 15, 2019 • edited Loading

sashadev-sky commented May 20, 2019

alaxalves commented May 20, 2019 • edited Loading

sashadev-sky commented May 20, 2019

alaxalves commented May 21, 2019 • edited Loading

kaustubh-nair commented May 21, 2019

jywarren commented May 22, 2019

alaxalves commented May 22, 2019

kaustubh-nair commented May 22, 2019

sashadev-sky commented May 23, 2019 • edited Loading

IgorWilbert May 24, 2019 • edited Loading

Choose a reason for hiding this comment

alaxalves May 27, 2019

Choose a reason for hiding this comment

IgorWilbert May 24, 2019

Choose a reason for hiding this comment

IgorWilbert May 24, 2019

Choose a reason for hiding this comment

IgorWilbert May 24, 2019

Choose a reason for hiding this comment

alaxalves May 24, 2019

Choose a reason for hiding this comment

sashadev-sky commented May 26, 2019

alaxalves commented May 27, 2019

sashadev-sky commented May 27, 2019

alaxalves commented May 27, 2019 • edited Loading

sashadev-sky commented May 27, 2019

alaxalves commented May 30, 2019 • edited Loading

kaustubh-nair commented May 31, 2019

alaxalves commented Jun 1, 2019 • edited Loading

alaxalves commented Jun 1, 2019

jywarren commented Jun 1, 2019

jywarren commented Jun 1, 2019

alaxalves commented Jun 2, 2019

alaxalves commented Apr 17, 2019 •

edited

Loading

kaustubh-nair commented Apr 18, 2019 •

edited

Loading

codecov bot commented Apr 19, 2019 •

edited

Loading

alaxalves commented May 15, 2019 •

edited

Loading

alaxalves commented May 20, 2019 •

edited

Loading

alaxalves commented May 21, 2019 •

edited

Loading

sashadev-sky commented May 23, 2019 •

edited

Loading

IgorWilbert May 24, 2019 •

edited

Loading

alaxalves commented May 27, 2019 •

edited

Loading

alaxalves commented May 30, 2019 •

edited

Loading

alaxalves commented Jun 1, 2019 •

edited

Loading