Code review #1
Code review #1
Conversation
| Frondit supports Ruby 2.2+, Rails. 4.1+ | ||
|
|
||
| ## FIXME | ||
| - Tests coverage is poor |
There was a problem hiding this comment.
Надо либо добавить тесты, либо написать что в инпрогресс либо не упоминать тесты вообще.
| @@ -1,30 +0,0 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Везде где можно нужны одинарные кавычки
| before_action action_list do | ||
| policies = gon.policies | ||
| policies ||= {} | ||
| policies[policy_class.to_s.camelize] = Frondit::Extractor.new(self, current_user, policy_class, config).call |
There was a problem hiding this comment.
Слишком длинная строка, 80 символов
There was a problem hiding this comment.
Интересно про current_user: он всегда есть там, куда миксуется модуль?
|
|
||
| EXCEPT_POLICY_METHODS = %i[user record].freeze | ||
|
|
||
| def initialize(controller_instance, user, policy_class, config) |
There was a problem hiding this comment.
Здесь бы подошли keyword arguments, например, так
initialize(controller_instance:, user:, policy_class:, config: {})
или
initialize(controller_instance:, user:, policy_class:, **config)
я считаю, так код нагляднее и более гибок для изменений.
Например, вот здесь мы не знаем, что значит первый аргумент:
Frondit::Extractor.new(self, current_user, policy_class, config).call
а так знаем:
extractor = Frondit::Extractor.new(
controller_instance: self,
user: current_user,
policy_class: policy_class,
config: config
)
policies[policy_class.to_s.camelize] = extractor.call
|
|
||
| private | ||
|
|
||
| attr_reader :controller_instance, :user, :policy_class_path, :rules_to_check, :record_variable_name |
There was a problem hiding this comment.
Я считаю, что объявления аттрибутов и аксессоров более читабельны, когда расположены наверху класса. Мы открываем класс и не листаем его в поисках аттрибутов. При этом видим сразу и паблик и приватные.
rubocop/ruby-style-guide#538 (comment)
attr_reader :controller_instance, :user, :policy_class_path, :rules_to_check, :record_variable_name
private :controller_instance, :user, :policy_class_path, :rules_to_check, :record_variable_name
| @policy_rules ||= policy_class.ancestors.map! do |klass| | ||
| next unless klass.to_s.include? "Policy" | ||
| klass.instance_methods(false).reject { |method| EXCEPT_POLICY_METHODS.include? method } | ||
| end.flatten.compact.uniq |
There was a problem hiding this comment.
Avoid do…end when chaining.
https://github.com/rubocop-hq/ruby-style-guide#single-line-blocks
| action_list[:only] = config[:on] if config[:on].present? | ||
| before_action action_list do | ||
| policies = gon.policies | ||
| policies ||= {} |
There was a problem hiding this comment.
Можно сократить
policies = gon.policies
policies ||= {}
до
policies = gon.policies || {}
| before_action action_list do | ||
| policies = gon.policies | ||
| policies ||= {} | ||
| policies[policy_class.to_s.camelize] = Frondit::Extractor.new(self, current_user, policy_class, config).call |
There was a problem hiding this comment.
Интересно про current_user: он всегда есть там, куда миксуется модуль?
| script_content = <<-SCRIPT | ||
| #{Gon::Base.render_data(need_tag: false)} | ||
| window.policies = function() { return window.gon.policies } | ||
| window.policy = function(name) { return window.gon.policies[name] } |
There was a problem hiding this comment.
я бы здесь выровняла отступы у =
| @user = user | ||
| @policy_class_path = policy_class | ||
| @rules_to_check = config[:only] | ||
| @record_variable_name = config[:record].to_sym if config[:record] |
There was a problem hiding this comment.
Это можно упростить @record_variable_name = config[:record].try(:to_sym)
No description provided.