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

Upgrade Rubocop/Erblint and fix cop violations #1803

Merged
merged 1 commit into from
Aug 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,19 @@ Rails:
Rails/FindEach:
Enabled: false

Rails/SkipsModelValidations:
Exclude:
- app/controllers/aip/v1/collections_controller.rb
Copy link
Contributor Author

@murny murny Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cop wants us to avoid using .insert because it skips validations. But it's a false positive as we using .insert from our rdf graph object instead of ActiveRecord.

app/controllers/aip/v1/collections_controller.rb:24:31: C: Rails/SkipsModelValidations: 
Avoid using insert because it skips validations. 
(https://guides.rubyonrails.org/active_record_validations.html#skipping-validations)

      rdf_graph_creator.graph.insert(RDF::Statement(statement_definition))
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So just disable this cop from these files

- app/controllers/aip/v1/entities_controller.rb
- app/services/rdf_graph_creation_service.rb

# There comes a point where I question Rubocop's maintainer's sanity
Rails/UnknownEnv:
Enabled: false

Style/ArrayCoercion:
Enabled: false

Style/AsciiComments:
Enabled: false

Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ and releases in Jupiter project adheres to [Semantic Versioning](http://semver.o
### Added
- tmp/cache to docker ignore [#1680](https://github.com/ualbertalib/jupiter/issues/1680)

### Fixed
- Upgrade Rubocop/Erblint and fix cop violations [#1803](https://github.com/ualbertalib/jupiter/pull/1803)

## [2.0.1.pre1] - 2020-07-22

### Added
Expand Down
4 changes: 2 additions & 2 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,13 @@ group :development, :test do
gem 'nokogiri'
gem 'selenium-webdriver', require: false

gem 'erb_lint', '>= 0.0.32', require: false
gem 'erb_lint', '>= 0.0.35', require: false

gem 'pry'
gem 'pry-byebug'
gem 'pry-rails'

gem 'rubocop', '~> 0.86.0', require: false
gem 'rubocop', '~> 0.89.0', require: false
gem 'rubocop-performance'
gem 'rubocop-rails'
end
Expand Down
55 changes: 30 additions & 25 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ GEM
brakeman (4.9.0)
builder (3.2.4)
bump (0.9.0)
byebug (11.1.1)
byebug (11.1.3)
capybara (3.33.0)
addressable
mini_mime (>= 0.1.3)
Expand All @@ -137,7 +137,7 @@ GEM
nap
open4 (~> 1.3)
clamby (1.6.6)
coderay (1.1.2)
coderay (1.1.3)
colored2 (3.1.2)
concurrent-ruby (1.1.6)
connection_pool (2.2.3)
Expand Down Expand Up @@ -168,13 +168,16 @@ GEM
activemodel-serializers-xml (>= 1.0)
activesupport (>= 5.0)
request_store (>= 1.0)
ebnf (1.2.0)
ebnf (2.1.1)
htmlentities (~> 4.3)
rdf (~> 3.1)
scanf (~> 1.0)
sxp (~> 1.1)
erb_lint (0.0.34)
erb_lint (0.0.35)
activesupport
better_html (~> 1.0.7)
html_tokenizer
parser (>= 2.7.1.4)
rainbow
rubocop (~> 0.79)
smart_properties
Expand All @@ -190,10 +193,10 @@ GEM
multipart-post (>= 1.2, < 3)
faraday-http-cache (2.2.0)
faraday (>= 0.8)
ffi (1.12.2)
fugit (1.3.4)
ffi (1.13.1)
fugit (1.3.8)
et-orbi (~> 1.1, >= 1.1.8)
raabro (~> 1.1)
raabro (~> 1.3)
git (1.7.0)
rchardet (~> 1.8)
globalid (0.4.2)
Expand All @@ -204,7 +207,8 @@ GEM
hashdiff (1.0.1)
hashie (3.6.0)
html_tokenizer (0.0.7)
i18n (1.8.3)
htmlentities (4.3.4)
i18n (1.8.5)
concurrent-ruby (~> 1.0)
image_processing (1.11.0)
mini_magick (>= 4.9.5, < 5)
Expand Down Expand Up @@ -295,7 +299,7 @@ GEM
nio4r (~> 2.0)
pundit (1.1.0)
activesupport (>= 3.0.0)
raabro (1.1.6)
raabro (1.3.1)
rack (2.2.3)
rack-protection (2.0.8.1)
rack
Expand Down Expand Up @@ -336,11 +340,11 @@ GEM
activesupport (>= 5.2.1)
i18n
polyamorous (= 2.3.2)
rb-fsevent (0.10.3)
rb-fsevent (0.10.4)
rb-inotify (0.10.1)
ffi (~> 1.0)
rchardet (1.8.0)
rdf (3.1.4)
rdf (3.1.5)
hamster (~> 3.0)
link_header (~> 0.0, >= 0.0.8)
rdf-aggregate-repo (3.1.0)
Expand All @@ -365,23 +369,23 @@ GEM
rsolr (2.3.0)
builder (>= 2.1.2)
faraday (>= 0.9.0)
rubocop (0.86.0)
rubocop (0.89.0)
parallel (~> 1.10)
parser (>= 2.7.0.1)
parser (>= 2.7.1.1)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 1.7)
rexml
rubocop-ast (>= 0.0.3, < 1.0)
rubocop-ast (>= 0.1.0, < 1.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 1.4.0, < 2.0)
rubocop-ast (0.2.0)
parser (>= 2.7.0.1)
rubocop-ast (0.3.0)
parser (>= 2.7.1.4)
rubocop-performance (1.7.1)
rubocop (>= 0.82.0)
rubocop-rails (2.6.0)
rubocop-rails (2.7.1)
activesupport (>= 4.2.0)
rack (>= 1.1)
rubocop (>= 0.82.0)
rubocop (>= 0.87.0)
ruby-progressbar (1.10.1)
ruby-saml (1.11.0)
nokogiri (>= 1.5.10)
Expand All @@ -395,6 +399,7 @@ GEM
sawyer (0.8.2)
addressable (>= 2.3.5)
faraday (> 0.8, < 2.0)
scanf (1.0.0)
sdoc (1.1.0)
rdoc (>= 5.0)
selenium-webdriver (3.142.7)
Expand Down Expand Up @@ -432,10 +437,10 @@ GEM
skylight-core (4.3.1)
activesupport (>= 4.2.0)
smart_properties (1.15.0)
sparql (3.1.0)
sparql (3.1.2)
builder (~> 3.2)
ebnf (~> 1.2)
rdf (~> 3.1)
ebnf (>= 1.1)
rdf (~> 3.1, >= 3.1.2)
rdf-aggregate-repo (~> 3.1)
rdf-xsd (~> 3.1)
sparql-client (~> 3.1)
Expand Down Expand Up @@ -482,14 +487,14 @@ GEM
rack-proxy (>= 0.6.1)
railties (>= 5.2)
semantic_range (>= 2.3.0)
websocket-driver (0.7.2)
websocket-driver (0.7.3)
websocket-extensions (>= 0.1.0)
websocket-extensions (0.1.5)
wicked (1.3.4)
railties (>= 3.0.7)
xpath (3.2.0)
nokogiri (~> 1.8)
zeitwerk (2.3.1)
zeitwerk (2.4.0)

PLATFORMS
ruby
Expand All @@ -512,7 +517,7 @@ DEPENDENCIES
danger (~> 8.0)
differ
draper
erb_lint (>= 0.0.32)
erb_lint (>= 0.0.35)
ezid-client (~> 1.8.0)
faker
haikunator
Expand Down Expand Up @@ -543,7 +548,7 @@ DEPENDENCIES
redis (~> 4.1)
rollbar
rsolr
rubocop (~> 0.86.0)
rubocop (~> 0.89.0)
rubocop-performance
rubocop-rails
rufus-scheduler (= 3.6.0)
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/facets/member_of_paths.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def display
community_id, collection_id = @value.split('/')
community_title = Community.find(community_id).title
collection_title = if collection_id
'/' + Collection.find(collection_id).title
"/#{Collection.find(collection_id).title}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop prefers string interpolation over string concatenation. Updates all our violations of this.

app/decorators/facets/member_of_paths.rb:10:26: C: 
Style/StringConcatenation: Prefer string interpolation to string concatenation. 
(https://rubystyle.guide#string-interpolation)
                         ‘/’ + Collection.find(collection_id).title
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

else
''
end
Expand Down
5 changes: 3 additions & 2 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@ def results_range(results)
def search_link_for(object, attribute, value: nil, facet: :facet, display: nil)
value ||= object.send(attribute)
display ||= value
if facet == :range_facet
case facet
when :range_facet
link_to(display, search_path(ranges: object.solr_exporter_class.facet_term_for(attribute,
value,
role: :range_facet)),
rel: 'nofollow')
elsif facet == :facet
when :facet
link_to(display, search_path(facets: object.solr_exporter_class.facet_term_for(attribute, value)),
rel: 'nofollow')
else
Expand Down
5 changes: 3 additions & 2 deletions app/models/draft_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,10 @@ def license_for_uri(uri)

# silly stuff needed for handling multivalued publication status attribute when Item type is `Article`
def publication_status_as_uri
if type.name == 'journal_article_draft'
case type.name
when 'journal_article_draft'
[CONTROLLED_VOCABULARIES[:publication_status].draft, CONTROLLED_VOCABULARIES[:publication_status].submitted]
elsif type.name == 'journal_article_published'
when 'journal_article_published'
[CONTROLLED_VOCABULARIES[:publication_status].published]
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/thesis.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ class Thesis < JupiterCore::Doiable
scope :updated_on_or_after, ->(date) { where('updated_at >= ?', date) }
scope :updated_on_or_before, ->(date) { where('updated_at <= ?', date) }

after_save :push_item_id_for_preservation
before_validation :populate_sort_year
after_save :push_item_id_for_preservation

validates :dissertant, presence: true
validates :graduation_date, presence: true
Expand Down
4 changes: 2 additions & 2 deletions app/policies/draft_item_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ def permitted_attributes
:source, :related_item,
:license, :license_text_area, :visibility, :embargo_end_date,
:status, :wizard_step,
language_ids: [], creators: [], subjects: [],
contributors: [], places: [], time_periods: [], citations: []]
{ language_ids: [], creators: [], subjects: [],
contributors: [], places: [], time_periods: [], citations: [] }]
end

end
2 changes: 1 addition & 1 deletion app/policies/draft_thesis_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def permitted_attributes
:rights, :visibility, :embargo_end_date,
:status, :wizard_step, :date_accepted, :date_submitted,
:degree, :degree_level, :institution_id, :specialization,
subjects: [], supervisors: [], departments: [], committee_members: []]
{ subjects: [], supervisors: [], departments: [], committee_members: [] }]
end

end
2 changes: 1 addition & 1 deletion config/environments/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
# config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')

if ENV['RAILS_LOG_TO_STDOUT'].present?
logger = ActiveSupport::Logger.new(STDOUT)
logger = ActiveSupport::Logger.new($stdout)
logger.formatter = config.log_formatter
config.logger = ActiveSupport::TaggedLogging.new(logger)
end
Expand Down
2 changes: 1 addition & 1 deletion config/environments/staging.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
# config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')

if ENV['RAILS_LOG_TO_STDOUT'].present?
logger = ActiveSupport::Logger.new(STDOUT)
logger = ActiveSupport::Logger.new($stdout)
logger.formatter = config.log_formatter
config.logger = ActiveSupport::TaggedLogging.new(logger)
end
Expand Down
2 changes: 1 addition & 1 deletion config/environments/uat.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
# config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')

if ENV['RAILS_LOG_TO_STDOUT'].present?
logger = ActiveSupport::Logger.new(STDOUT)
logger = ActiveSupport::Logger.new($stdout)
logger.formatter = config.log_formatter
config.logger = ActiveSupport::TaggedLogging.new(logger)
end
Expand Down
4 changes: 2 additions & 2 deletions lib/tasks/batch_ingest.rake
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def thesis_ingest(thesis_data, index, csv_directory, checksums)
graduation_term_string = graduation_term_array[0]
graduation_term = '11' if graduation_term_string == 'Fall'
graduation_term = '06' if graduation_term_string == 'Spring'
unlocked_obj.graduation_date = graduation_year + '-' + graduation_term
unlocked_obj.graduation_date = "#{graduation_year}-#{graduation_term}"
end
unlocked_obj.abstract = thesis_data[:abstract]

Expand Down Expand Up @@ -284,7 +284,7 @@ end
def generate_checksums(csv_directory)
require 'digest/md5'
checksums = {}
Dir.glob(csv_directory + '/*.pdf').each do |f|
Dir.glob("#{csv_directory}/*.pdf").each do |f|
checksum = Digest::MD5.hexdigest(File.read(f))
checksums[checksum] = File.basename(f)
end
Expand Down
Loading