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

Refactor with/against Rubocop #405

Closed
wants to merge 119 commits into from
Closed

Refactor with/against Rubocop #405

wants to merge 119 commits into from

Conversation

ZJvandeWeg
Copy link
Contributor

In response to #402. I've implemented Rubocop for this project.

Created this PR in case someone wants to help out?

@rakoo
Copy link

rakoo commented Mar 14, 2015

For reference, here's a list of all style errors found in 731.1, with an example for each one:

lib/sup/source.rb:164:1: C: Style/AccessModifierIndentation: Indent access modifiers like protected.
protected
^^^^^^^^^
lib/sup/index.rb:113:7: C: Style/AccessorMethodName: Do not prefix reader method names with get_.
  def get_xapian
      ^^^^^^^^^^
lib/sup/index.rb:874:3: C: Style/Alias: Use alias_method instead of alias.
  alias old_add_term add_term
  ^^^^^
lib/sup/crypto.rb:86:9: C: Style/AlignArray: Align the elements of an array literal if they span more than one line.
        'Install the gpgme gem in order to use signed and encrypted emails']
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/message_chunks.rb:134:25: C: Style/AlignHash: Align the elements of a hash literal if they span more than one line.
                        :filename => lambda { write_to_disk },
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/crypto.rb:101:32: C: Style/AlignParameters: Align the parameters of a method call if they span more than one line.
                               {:operation => "sign", :options => {}}) || {}
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/search.rb:17:32: C: Style/AndOr: Use || instead of or.
        l =~ /^([^:]*): (.*)$/ or raise "can't parse #{fn} line #{l.inspect}"
                               ^^
lib/sup/message.rb:671:9: C: Style/AsciiComments: Use only ascii symbols in comments.
        # look for next nonblank line only when needed to avoid O(n²)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/maildir.rb:133:33: C: Style/BlockEndNewline: Expression at 133, 33 should be on its own line.
          |f| !File.directory? f}.map {
                                ^
lib/sup/modes/thread_index_mode.rb:1002:60: C: Style/Blocks: Avoid using {...} for multi-line blocks.
      (t.labels - @hidden_labels).sort_by {|x| x.to_s}.map {
                                                           ^
lib/sup/crypto.rb:69:33: C: Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
            GPGME.check_version({:protocol => GPGME::PROTOCOL_OpenPGP})
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/util/ncurses.rb:46:48: C: Style/CaseEquality: Avoid the use of the case equality operator ===.
      if status == Ncurses::ERR || c.nil? || c === Ncurses::ERR
                                               ^^^
lib/sup/person.rb:105:9: C: Style/CaseIndentation: Indent when as deep as case.
        when /(.+?) ((\S+?)@\S+) \3/
        ^^^^
lib/sup/util.rb:330:12: C: Style/CharacterLiteral: Do not use the character literal - use string literal instead.
      when ?"
           ^^
lib/sup/index.rb:858:7: C: Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.
class Xapian::Document
      ^^^^^^^^^^^^^^^^
lib/sup/account.rb:53:95: C: Style/ClassCheck: Prefer Object#is_a? over Object#kind_of?.
    fail "alternative emails are not an array: #{hash[:alternates]}" unless hash[:alternates].kind_of? Array
                                                                                              ^^^^^^^^
lib/sup/source.rb:169:7: C: Style/ClassMethods: Use self.expand_filesystem_uri instead of Source.expand_filesystem_uri.
  def Source.expand_filesystem_uri uri
      ^^^^^^
lib/sup/undo.rb:14:5: C: Style/ClassVars: Replace class var @@actionlist with a class instance var.
    @@actionlist = []
    ^^^^^^^^^^^^
lib/sup/source.rb:227:27: C: Style/ColonMethodCall: Do not use :: for method calls.
    source_array = Redwood::load_yaml_obj(fn) || []
                          ^^
lib/sup/index.rb:210:7: C: Style/CommentAnnotation: Annotation keywords should be all upper case, followed by a colon and a space, then a note describing the problem.
    # TODO thread by subject
      ^^^^
lib/sup/person.rb:30:1: C: Style/CommentIndentation: Incorrect indentation detected (column 0 instead of 2).
#   alias :eql? :==
^^^^^^^^^^^^^^^^^^^
lib/sup/search.rb:48:29: C: Style/DeprecatedHashMethods: Hash#has_key? is deprecated in favor of Hash#key?.
    if @predefined_searches.has_key? name
                            ^^^^^^^^
lib/sup/sent.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class SentManager
^^^^^
lib/sup/util/ncurses.rb:123:98: C: Style/DotPosition: Place the . on the next line, together with the method name.
      [ :"[]=", :"<<", :replace, :insert, :prepend, :append, :concat, :force_encoding, :setbyte ].
                                                                                                 ^
lib/sup/util/ncurses.rb:136:29: C: Style/DoubleNegation: Avoid the use of double negation (!!).
        superclass.dumb? or !!@dumb
                            ^
lib/sup/modes/edit_message_mode.rb:456:49: C: Style/EachWithObject: Use each_with_object instead of inject.
      header = Source.parse_raw_email_header(f).inject({}) { |h, (k, v)| h[k.capitalize] = v; h } # lousy HACK
                                                ^^^^^^
lib/sup/message.rb:95:5: C: Style/ElseAlignment: Align else with if.
    else
    ^^^^
lib/sup/util.rb:360:7: C: Style/EmptyElse: Redundant empty else-clause.
      else
      ^^^^
lib/sup/sent.rb:50:3: C: Style/EmptyLineBetweenDefs: Use empty lines between defs.
  def uri; 'sup://sent' end
  ^^^
lib/sup/sent.rb:2:1: C: Style/EmptyLinesAroundModuleBody: Extra empty line detected at module body beginning.
lib/sup/sent.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class SentManager
lib/sup/service/label_service.rb:27:5: C: Style/EmptyLinesAroundAccessModifier: Keep a blank line before and after private.
    private
    ^^^^^^^
lib/sup/person.rb:81:1: C: Style/EmptyLinesAroundClassBody: Extra empty line detected at class body beginning.
lib/sup/person.rb:82:22: C: Style/MethodDefParentheses: Use def with parentheses when there are parameters.
    def full_address name, email
lib/sup/poll.rb:88:1: C: Style/EmptyLinesAroundMethodBody: Extra empty line detected at method body end.
lib/sup/poll.rb:98:13: C: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
      debug "poll already in progress."
lib/sup/sent.rb:2:1: C: Style/EmptyLinesAroundModuleBody: Extra empty line detected at module body beginning.
lib/sup/sent.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class SentManager
lib/sup/util.rb:699:13: C: Style/EmptyLiteral: Use hash literal {} instead of Hash.new.
    @hash = Hash.new
            ^^^^^^^^
lib/sup/util.rb:606:26: C: Style/FlipFlop: Avoid the use of flip flop operators.
    select { |l| true if l == startline .. l == endline }
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/util.rb:35:70: C: Style/FormatString: Favor format over String#%.
      "host: %s\npid: %s\nppid: %s\ntime: %s\nuser: %s\npname: %s\n" %
                                                                     ^
lib/sup/sent.rb:44:40: C: Style/GlobalVars: Do not introduce global variables.
    super "mbox://" + @filename, true, $config[:archive_sent]
                                       ^^^^^^^
lib/sup/util/ncurses.rb:223:5: C: Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression.
    if not defined? Ncurses.get_wch
    ^^
lib/sup/search.rb:24:53: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    @predefined_queries  = { 'All mail'.to_sym => { :qobj => Xapian::Query.new('Kmail'),
                                                    ^^^^^^^^
lib/sup/search.rb:38:5: C: Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
    if @predefined_searches.keys.member? name
    ^^
lib/sup/modes/thread_index_mode.rb:998:7: C: Style/IndentArray: Use 2 spaces for indentation in an array, relative to the start of the line where the left bracket is.
      [:size_widget_color, size_widget_text],
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/hook.rb:118:5: C: Style/IndentationConsistency: Inconsistent indentation detected.
    HookManager.descs.sort.each do |name, desc|
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/sent.rb:3:1: C: Style/IndentationWidth: Use 2 (not 0) spaces for indentation.
class SentManager

lib/sup/index.rb:82:7: C: Style/InfiniteLoop: Use Kernel#loop for infinite loops.
      while true
      ^^^^^
lib/sup/index.rb:165:37: C: Style/Lambda: Use the new lambda literal syntax -> {...}.
    each_id(query) { |id| yield id, lambda { build_message id } }
                                    ^^^^^^
lib/sup/util/ncurses.rb:263:9: C: Style/LeadingCommentSpace: Missing space after #.
        #c.is_a?(Fixnum) ? c : c.ord
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/util.rb:306:25: C: Style/MethodCallParentheses: Do not use parentheses for method calls with no arguments.
    normalize_whitespace().split(/,\s*(?=(?:[^"]*"[^"]*")*(?![^"]*"))/)
                        ^
lib/sup/sent.rb:8:18: C: Style/MethodDefParentheses: Use def with parentheses when there are parameters.
  def initialize source_uri
                 ^^^^^^^^^^
lib/sup/modes/thread_index_mode.rb:517:7: C: Style/MultilineBlockChain: Avoid multi-line chains of blocks.
      end.each do |t|
      ^^^^^^^^
lib/sup/modes/thread_index_mode.rb:1003:13: C: Style/MultilineBlockLayout: Block argument expression is not on the same line as the block start.
            |label| [Colormap.sym_is_defined("label_#{label}_color".to_sym) || :label_color, "#{label} "]
            ^^^^^^^
lib/sup/message_chunks.rb:197:70: C: Style/MultilineIfThen: Do not use then for multi-line if.
        tempname = if (File.extname @filename) =~ /^\.[[:alnum:]]+$/ then
                                                                     ^^^^
lib/sup/util/ncurses.rb:124:7: C: Style/MultilineOperationIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
      select{ |m| public_method_defined?(m) }.
      ^^^^^^
lib/sup/util/ncurses.rb:202:5: C: Style/NegatedIf: Favor unless over if for negative conditions.
    if not defined? Form.form_driver_w
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/index.rb:185:5: C: Style/NegatedWhile: Favor until over while for negative conditions.
    while not queue.empty?
    ^^^^^^^^^^^^^^^^^^^^^^
lib/sup/modes/thread_index_mode.rb:965:47: C: Style/NestedTernaryOperator: Ternary operators must not be nested. Prefer if/else constructs instead.
      from << [(newness ? :index_new_color : (starred ? :index_starred_color : :index_old_color)), abbrev]
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/message.rb:669:11: C: Style/Next: Use next to skip iteration.
    lines.each_with_index do |line, i|
          ^^^^^^^^^^^^^^^
lib/sup/index.rb:689:19: C: Style/NonNilCheck: Prefer !expression.nil? over expression != nil.
    existed = doc != nil
                  ^^
lib/sup/util/ncurses.rb:112:27: C: Style/Not: Use ! instead of not.
    def present?        ; not empty?                                end  ## Proxy method
                          ^^^
lib/sup/message.rb:553:52: C: Style/NumericLiterals: Separate every 3 digits in the integer portion of a number with underscores(_).
          ["sup-attachment-#{Time.now.to_i}-#{rand 10000}", extension].join(".")
                                                   ^^^^^
lib/sup/modes/completion_mode.rb:24:13: C: Style/OneLineConditional: Favor the ternary operator (?:) over if/then/else/end constructs.
  def roll; if at_bottom? then jump_to_start else page_down end end
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/source.rb:75:10: C: Style/OpMethod: When defining the == operator, name its argument other.
  def == o; o.uri == uri; end
         ^
lib/sup/modes/edit_message_mode.rb:220:23: C: Style/ParenthesesAroundCondition: Don't use parentheses around the condition of an if.
    @file.puts sig if ($config[:edit_signature] and !@sig_edited)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/index.rb:389:21: C: Style/PercentLiteralDelimiters: %w-literals should be delimited by ( and )
  COMPL_OPERATORS = %w[AND OR NOT]
                    ^^^^^^^^^^^^^^
lib/sup/search.rb:18:19: C: Style/PerlBackrefs: Avoid the use of Perl-style backrefs.
        @searches[$1] = $2
                  ^^
lib/sup/util/ncurses.rb:101:9: C: Style/PredicateName: Rename is_keycode? to keycode?.
    def is_keycode?(c)  ; keycode?   &&  code == c                  end  ## Tests if keycode matches
        ^^^^^^^^^^^
lib/sup/sent.rb:16:5: C: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
    raise FatalSourceError.new("Configured sent_source [#{s.uri}] can't store mail.  Correct your configuration.") unless s.respond_to? :store_message
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/util/ncurses.rb:56:7: C: Style/RedundantBegin: Redundant begin block detected.
      begin
      ^^^^^
lib/sup/search.rb:35:27: C: Style/RedundantReturn: Redundant return detected.
  def predefined_queries; return @predefined_queries; end
                          ^^^^^^
lib/sup/person.rb:134:40: C: Style/RedundantSelf: Redundant self detected.
      ss.dup.split_on_commas.map { |s| self.from_address s }
                                       ^^^^^^^^^^^^^^^^^^^
lib/sup/crypto.rb:350:37: C: Style/RegexpLiteral: Use %r only for regular expressions matching more than 1 '/' character.
      if msg.header.content_type =~ %r{^multipart/} && !msg.multipart?
                                    ^^^^^^^^^^^^^^^
lib/sup/index.rb:778:39: C: Style/RescueModifier: Avoid using rescue in its modifier form.
    old_entry[:locations].each { |x| (doc.remove_term mkterm(:location, *x) rescue nil) } if old_entry
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/textfield.rb:124:11: C: Style/SelfAssignment: Use self-assignment shorthand +=.
          @i = @i + (c.is_keycode?(Ncurses::KEY_UP) ? -1 : 1)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/search.rb:37:29: C: Style/Semicolon: Do not use semicolons to terminate expressions.
  def search_string_for name;
                            ^
lib/sup/sent.rb:16:5: C: Style/SignalException: Use fail instead of raise to signal exceptions.
    raise FatalSourceError.new("Configured sent_source [#{s.uri}] can't store mail.  Correct your configuration.") unless s.respond_to? :store_message
    ^^^^^
lib/sup/colormap.rb:179:32: C: Style/SingleLineBlockParams: Name inject block params |a, e|.
    color = attrs.inject(cp) { |color, attr| color | attr }
                               ^^^^^^^^^^^^^
lib/sup/sent.rb:13:3: C: Style/SingleLineMethods: Avoid single-line method definitions.
  def source_id; @source.id; end
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/search.rb:30:36: C: Style/SpaceAfterComma: Space missing after comma.
    @predefined_searches.each do |k,v|
                                   ^
lib/sup/source.rb:134:5: C: Style/SpaceAfterControlKeyword: Use space after control keywords.
    while(line = f.gets)
    ^^^^^
lib/sup/index.rb:398:11: C: Style/SpaceAroundBlockParameters: Space after closing | missing.
  ).map{|p|"#{p}:"} + COMPL_OPERATORS
          ^
lib/sup/util/ncurses.rb:55:34: C: Style/SpaceAroundEqualsInParameterDefault: Surrounding space missing in default value assignment.
    def self.get handle_interrupt=true
                                 ^
lib/sup/crypto.rb:448:83: C: Style/SpaceAroundOperators: Surrounding space missing for operator '*'.
        output_lines << "Full fingerprint is: " + (0..9).map {|i| signature.fpr[(i*4),4]}.join(":")
                                                                                  ^
lib/sup/util/ncurses.rb:124:13: C: Style/SpaceBeforeBlockBraces: Space missing to the left of {.
      select{ |m| public_method_defined?(m) }.
            ^
lib/sup/modes/thread_index_mode.rb:999:30: C: Style/SpaceBeforeComma: Space found before comma.
      [:with_attachment_color , t.labels.member?(:attachment) ? "@" : " "],
                             ^
lib/sup/modes/thread_view_mode.rb:473:24: C: Style/SpaceBeforeComment: Put a space before an end-of-line comment.
      jump_to_message m#, true
                       ^^^^^^^
lib/sup/util/ncurses.rb:97:21: C: Style/SpaceBeforeSemicolon: Space found before semicolon.
    def to_character    ; character? ? self : "<#{code}>"           end  ## Returns character or code as a string
                    ^^^^
lib/sup/sent.rb:43:32: C: Style/SpaceInsideBlockBraces: Space inside empty braces detected.
    File.open(@filename, "w") { } unless File.exist? @filename
                               ^
lib/sup/util/ncurses.rb:123:8: C: Style/SpaceInsideBrackets: Space inside square brackets detected.
      [ :"[]=", :"<<", :replace, :insert, :prepend, :append, :concat, :force_encoding, :setbyte ].
       ^
lib/sup/search.rb:28:80: C: Style/SpaceInsideHashLiteralBraces: Space inside } missing.
                                                    :text => 'Search all mail.'}
                                                                               ^
lib/sup/modes/reply_mode.rb:75:96: C: Style/SpaceInsideParens: Space inside parentheses detected.
    elsif(b = (@m.to.collect {|t| t.email} + @m.cc.collect {|c| c.email} + [@m.recipient_email] ).find { |p| AccountManager.is_account_email? p })
                                                                                               ^
lib/sup/message.rb:619:38: C: Style/SpaceInsideRangeLiteral: Space inside range literal.
      before = startidx != 0 ? lines[0 .. startidx-1] : []
                                     ^^^^^^^^^^^^^^^
lib/sup/index.rb:604:20: C: Style/SpecialGlobalVars: Prefer $ERROR_INFO from the English library over $!.
      raise unless $!.message =~ /DocNotFoundError/
                   ^^
lib/sup/sent.rb:29:13: C: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
      debug "store the sent message (locking sent source..)"
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/hook.rb:121:3: C: Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.
#{"-" * name.length}
  ^^^
lib/sup/source.rb:180:34: C: Style/SymbolProc: Pass &:to_s as an argument to map instead of a block.
    c.instance_eval { @labels = (@labels.to_a.map { |l| l.to_s }).sort }
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/message.rb:525:7: C: Style/Tab: Tab detected.
        chunks = inline_gpg_to_chunks m.body, $encoding, (m.charset || $encoding)
      ^
lib/sup/util.rb:747:1: C: Style/TrailingBlankLines: 1 trailing blank lines detected.
lib/sup/account.rb:2:1: C: Style/EmptyLinesAroundModuleBody: Extra empty line detected at module body beginning.
lib/sup/account.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
lib/sup/index.rb:367:62: C: Style/TrailingComma: Avoid comma after the last item of a hash.
    '' => {:prefix => %w(S B FN TN A E), :exclusive => false},
                                                             ^
lib/sup/util/ncurses.rb:111:89: C: Style/TrailingWhitespace: Trailing whitespace detected.
    def keycode=(c)     ; replace(c); keycode! ; self               end  ## Sets keycode    
                                                                                        ^^^^
lib/sup/sent.rb:47:3: C: Style/TrivialAccessors: Use attr_reader to define trivial reader methods.
  def file_path; @filename end
  ^^^
lib/sup/undo.rb:25:5: C: Style/UnlessElse: Do not use unless with else. Rewrite these with the positive case first.
    unless @@actionlist.empty?
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/sup/person.rb:23:9: C: Style/VariableInterpolation: Replace interpolated variable @name with expression #{@name}.
      "#@name <#@email>"
        ^^^^^
lib/sup/index.rb:339:5: C: Style/VariableName: Use snake_case for variable names.
    lastTerm = @xapian._dangerous_allterms_end prefix
    ^^^^^^^^
lib/sup/util/locale_fiddler.rb:12:32: C: Style/WhenThen: Do not use when x;. Use when x then instead.
                  when /darwin/; "libc.dylib"
                               ^
lib/sup/index.rb:592:5: C: Style/WhileUntilModifier: Favor modifier while usage when having a single-line body.
    while docid > 0 and docid_exists? docid
    ^^^^^
lib/sup/modes/thread_view_mode.rb:781:21: C: Style/WordArray: Use %w or %W for array of words.
        next unless ["http", "https"].include?(u.scheme)
                    ^^^^^^^^^^^^^^^^^

@rakoo
Copy link

rakoo commented Mar 14, 2015

Some comments on the rules I don't totally agree with:

  • Style/AndOr: I actually like prose as defined in HACKING: use || and && for tests, use and and or for control flow. But it seems it's against the good style both from GH and rubocop
  • Style/AsciiComments: this contradicts the Use UTF-8 everywhere rule. I'm ok with UTF-8 comments
  • Style/EmptyLineBetweenDefs: I'm ok with not having spaces between defs, particularly when they are single line
  • Style/HashSyntax: GH says "use hash rockets", rubocop says "Use new syntax"... I'm ok with both (not at the same time of course)
  • Style/MethodDefParentheses: I'm ok with not having parentheses when arguments are "simple"
  • Style/MultilineBlockChain: What's the alternative ? Creating intermediate variables ? I'm not sure I like it
  • Style/SingleLineMethods: I do like single line methods when they're simple

Thanks for starting this !

@ZJvandeWeg
Copy link
Contributor Author

No problem! I agree with you on most things. Also, what happens if Rubocop changes policy later on, is sup then refactored again?

When I opened #402 I didn't expect there to be over 5k violations. Im wondering if it wouldn't be a better approach to disable all rules and then incrementely enable rules?

@maxmeyer
Copy link

@ZJvandeWeg

over 5k violations

😃 Sorry for not mentioning that this is very likely to happen. But ....

approach to disable all rules and then incrementely enable rules?

This is possible. I suggest to fix those violations incremental by:

  1. Run rubocop with --auto-gen-config

    This will generate a configuration file which disables all violoations and modify all rubocop configuration values to "suppress" violations. To enable a "cop" just remove the entry from the file and the violation occures again.

    rubocop --auto-gen-config
    

    This will create a "todo" file.

  2. Include "todo"-file in .rubocop.yml

    See the man page for more information about this.

    inherit_from: .rubocop_todo.yml
  3. Enable cop

    Remove an entry from the file.

  4. Run rubocop

    If rubocop provides an autocorrection for the violation use the following code snippet:

    rubocop --auto-correct

    This works quite good. I've never found, that this introduced a bug in my code.

  5. Fix the violation

    If no autocorrection is available, you need to fix it by hand. If you disagree with a violation and don't want to disable it globally place rubocop:disable and rubocop:enabl around the code block.

    # rubocop:disable Style/MethodLength
    def my_method
      [...]
    end
    # rubocop:enable Style/MethodLength
    
  6. Run test suite

    Just to make sure not bug was introduced by the refactoring, run the test suite

  7. Commit change

    Add a commit for the change. This gives you a nice overview what was fixed by rubocop/you

  8. Back to 3.

Cheers,
/mm

@maxmeyer
Copy link

⚠️ I strongly recommend to use the --auto-correct-switch a lot. Otherwise it is a lot of work to fix violations, especially the whitespace ones.

@maxmeyer
Copy link

BTW: Nice to see sup is improving. 😄 I use it a lot.

@ZJvandeWeg
Copy link
Contributor Author

I think ill stop at this point, as the rest requires major refactoring of the source instead of minor edits.
Soon ill run this branch at home, combined with the rmail branch to detect any bugs.

@terotil
Copy link
Contributor

terotil commented Mar 31, 2015

@rakoo wrote

Style/MultilineBlockChain: What's the alternative ? Creating intermediate variables ? I'm not sure I like it

In many cases I'd expect that extracting blocks as methods would clean the source in addition to getting rind of the style violations. That way you also get to give a name for the operation the piece of code does and thus ad semantics.

@ZJvandeWeg ZJvandeWeg closed this Jul 8, 2022
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.

5 participants