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

Cleanup and make misc files compliant with Rubocop. #4940

Merged
merged 1 commit into from Jun 6, 2016

Conversation

Projects
None yet
4 participants
@DirtyF
Member

DirtyF commented May 24, 2016

#4885

Ruby noob here.
There are still some warning left, I don't know how to fix.
Hope it helps though.

Show outdated Hide outdated lib/jekyll/collection.rb Outdated
relative_dir = Jekyll.sanitized_path(relative_directory,
File.dirname(file_path)).chomp("/.")
files << StaticFile.new(site, site.source, relative_dir,
File.basename(full_path), self)

This comment has been minimized.

@envygeeks

envygeeks May 24, 2016

Contributor

If a short line like this has to be sent to a new line our new trial is to try and collapse the class, (assuming since you made the "noob comment") if you look at the top you can see all those classes so collapse them from:

module Jekyll
  class MyClass1
    class MyClass2
      class MyClass3
      end
    end
  end
end

Into:

class Jekyll::MyClass1::MyClass2
   class MyClass3
   end
end

And then go from there and see how much you have to shrink. We lose a lot of space in Jekyll so we are trying to regain that space to keep readability high, at least this is what I remember being agreed upon yeah @jekyll/core?

@envygeeks

envygeeks May 24, 2016

Contributor

If a short line like this has to be sent to a new line our new trial is to try and collapse the class, (assuming since you made the "noob comment") if you look at the top you can see all those classes so collapse them from:

module Jekyll
  class MyClass1
    class MyClass2
      class MyClass3
      end
    end
  end
end

Into:

class Jekyll::MyClass1::MyClass2
   class MyClass3
   end
end

And then go from there and see how much you have to shrink. We lose a lot of space in Jekyll so we are trying to regain that space to keep readability high, at least this is what I remember being agreed upon yeah @jekyll/core?

Show outdated Hide outdated lib/jekyll/configuration.rb Outdated
Show outdated Hide outdated lib/jekyll/configuration.rb Outdated
Show outdated Hide outdated lib/jekyll/configuration.rb Outdated
Show outdated Hide outdated lib/jekyll/configuration.rb Outdated
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 24, 2016

Contributor

Thanks! Just a few comments.

Contributor

envygeeks commented May 24, 2016

Thanks! Just a few comments.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF May 25, 2016

Member

Thanks @envygeeks for your help, there are 7 offenses left that needs to be addressed by an experienced ruby developer:

lib/jekyll/collection.rb:55:5: C: Metrics/AbcSize: Assignment Branch Condition size for read is too high. [31.4/20]
    def read
    ^^^
lib/jekyll/configuration.rb:202:5: C: Metrics/AbcSize: Assignment Branch Condition size for backwards_compatibilize is too high. [39.37/20]
    def backwards_compatibilize
    ^^^
lib/jekyll/configuration.rb:202:5: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for backwards_compatibilize is too high. [10/8]
    def backwards_compatibilize
    ^^^
lib/jekyll/configuration.rb:202:5: E: Metrics/MethodLength: Method has too many lines. [50/20]
    def backwards_compatibilize ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/jekyll/configuration.rb:202:5: C: Metrics/PerceivedComplexity: Perceived complexity for backwards_compatibilize is too high. [10/8]
    def backwards_compatibilize
    ^^^
lib/jekyll/configuration.rb:328:9: C: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
        raise ArgumentError.new("Configuration file: (INVALID) #{file}".yellow)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/jekyll/deprecator.rb:26:7: C: Style/NegatedIf: Favor unless over if for negative conditions.
      if args.!empty? && args.first =~ /^--/ && !%w(--help --version).include?(args.first) ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Member

DirtyF commented May 25, 2016

Thanks @envygeeks for your help, there are 7 offenses left that needs to be addressed by an experienced ruby developer:

lib/jekyll/collection.rb:55:5: C: Metrics/AbcSize: Assignment Branch Condition size for read is too high. [31.4/20]
    def read
    ^^^
lib/jekyll/configuration.rb:202:5: C: Metrics/AbcSize: Assignment Branch Condition size for backwards_compatibilize is too high. [39.37/20]
    def backwards_compatibilize
    ^^^
lib/jekyll/configuration.rb:202:5: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for backwards_compatibilize is too high. [10/8]
    def backwards_compatibilize
    ^^^
lib/jekyll/configuration.rb:202:5: E: Metrics/MethodLength: Method has too many lines. [50/20]
    def backwards_compatibilize ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/jekyll/configuration.rb:202:5: C: Metrics/PerceivedComplexity: Perceived complexity for backwards_compatibilize is too high. [10/8]
    def backwards_compatibilize
    ^^^
lib/jekyll/configuration.rb:328:9: C: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
        raise ArgumentError.new("Configuration file: (INVALID) #{file}".yellow)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/jekyll/deprecator.rb:26:7: C: Style/NegatedIf: Favor unless over if for negative conditions.
      if args.!empty? && args.first =~ /^--/ && !%w(--help --version).include?(args.first) ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@DirtyF DirtyF changed the title from Rubocop : collections, command, configuration to Rubocop cleanup for misc files May 25, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 25, 2016

Contributor

This is ready to go, I vote let it fail so we are forced to look into it over this weekend and fix these problems or adjust Rubocop, but what @DirtyF did is complete and LGTM.

:shipit:

Contributor

envygeeks commented May 25, 2016

This is ready to go, I vote let it fail so we are forced to look into it over this weekend and fix these problems or adjust Rubocop, but what @DirtyF did is complete and LGTM.

:shipit:

@envygeeks envygeeks added the shipit label May 25, 2016

Show outdated Hide outdated lib/jekyll/deprecator.rb Outdated
Show outdated Hide outdated lib/jekyll/deprecator.rb Outdated
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

_Needs Rebase_

Contributor

envygeeks commented Jun 2, 2016

_Needs Rebase_

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 3, 2016

Member

@envygeeks seems like I got the rebase wrong 😠 and messed it all up 😞

Member

DirtyF commented Jun 3, 2016

@envygeeks seems like I got the rebase wrong 😠 and messed it all up 😞

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 3, 2016

Contributor

@DirtyF probably not you, we had a lot of merges go through today! You should be able to rebase -i and fix it no? If you can't feel free to let us know the error and we'll happily help.

Contributor

envygeeks commented Jun 3, 2016

@DirtyF probably not you, we had a lot of merges go through today! You should be able to rebase -i and fix it no? If you can't feel free to let us know the error and we'll happily help.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 6, 2016

Member

@envygeeks FYI here is the message displayed when I try to finish to rebase:

~/code/jekyll feature-rubocop|rebase
❯ git status
rebase in progress; onto 1f01c88
You are currently rebasing branch 'feature-rubocop' on '1f01c88'.
  (all conflicts fixed: run "git rebase --continue")

nothing to commit, working directory clean
VCS_INFO_get_data_git:223: no such file or directory: .git/rebase-apply/msg-clean

❯ git rebase --continue
fatal: cannot resume: .git/rebase-apply/final-commit does not exist.
VCS_INFO_get_data_git:223: no such file or directory: .git/rebase-apply/msg-clean
Member

DirtyF commented Jun 6, 2016

@envygeeks FYI here is the message displayed when I try to finish to rebase:

~/code/jekyll feature-rubocop|rebase
❯ git status
rebase in progress; onto 1f01c88
You are currently rebasing branch 'feature-rubocop' on '1f01c88'.
  (all conflicts fixed: run "git rebase --continue")

nothing to commit, working directory clean
VCS_INFO_get_data_git:223: no such file or directory: .git/rebase-apply/msg-clean

❯ git rebase --continue
fatal: cannot resume: .git/rebase-apply/final-commit does not exist.
VCS_INFO_get_data_git:223: no such file or directory: .git/rebase-apply/msg-clean
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 6, 2016

Contributor

@DirtyF do you have Atom editor? If you do then there is a great plugin callled "merge conflicts" that can help you: https://atom.io/packages/merge-conflicts (it's what I use.) The problem here is that when you get that message it ran into a block of code that changed and it can't automatically figure it out, what you'll have to do is edit the conflicted files and select which chunk you want (merge-conflicts can help you there) and then git add them and then git rebase --continue.

Contributor

envygeeks commented Jun 6, 2016

@DirtyF do you have Atom editor? If you do then there is a great plugin callled "merge conflicts" that can help you: https://atom.io/packages/merge-conflicts (it's what I use.) The problem here is that when you get that message it ran into a block of code that changed and it can't automatically figure it out, what you'll have to do is edit the conflicted files and select which chunk you want (merge-conflicts can help you there) and then git add them and then git rebase --continue.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 6, 2016

Member

@envygeeks yup, that's exactly what I did. The plugin does not detect any confict anymore.

❯ git status
rebase in progress; onto 1f01c88
You are currently rebasing branch 'feature-rubocop' on '1f01c88'.
  (all conflicts fixed: run "git rebase --continue")

nothing to commit, working directory clean
❯ git rebase --continue
fatal: cannot resume: .git/rebase-apply/author-script does not exist.
Member

DirtyF commented Jun 6, 2016

@envygeeks yup, that's exactly what I did. The plugin does not detect any confict anymore.

❯ git status
rebase in progress; onto 1f01c88
You are currently rebasing branch 'feature-rubocop' on '1f01c88'.
  (all conflicts fixed: run "git rebase --continue")

nothing to commit, working directory clean
❯ git rebase --continue
fatal: cannot resume: .git/rebase-apply/author-script does not exist.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 6, 2016

Contributor

@DirtyF there are two options at that point, touch the file and see if continue works at that point (a friend who teaches git said it _should_ be no problem.) Or git rebase --abort and make sure that there are no processes holding something and do it again.

Contributor

envygeeks commented Jun 6, 2016

@DirtyF there are two options at that point, touch the file and see if continue works at that point (a friend who teaches git said it _should_ be no problem.) Or git rebase --abort and make sure that there are no processes holding something and do it again.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 6, 2016

Member

@envygeeks I finally managed to rebase and push --force my branch (tell me if I better clean with interactive mode).

Member

DirtyF commented Jun 6, 2016

@envygeeks I finally managed to rebase and push --force my branch (tell me if I better clean with interactive mode).

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 6, 2016

Contributor

@DirtyF yes it would be great if you could rebase into a single commit! Looks like those two errors are easy to fix too, once you do that I can look quickly and see why cucumber is failing on your pull.

Contributor

envygeeks commented Jun 6, 2016

@DirtyF yes it would be great if you could rebase into a single commit! Looks like those two errors are easy to fix too, once you do that I can look quickly and see why cucumber is failing on your pull.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 6, 2016

Member

@envygeeks done. There are no offenses left now 😓

Member

DirtyF commented Jun 6, 2016

@envygeeks done. There are no offenses left now 😓

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 6, 2016

Contributor

Hulk here,

I'm watching for you and I've restarted a test that randomly failed @DirtyF. @envygeeks So far no cases have replicated what you had me watching for. This is ready @envygeeks, it seems you were wrong, thanks for waking me up so early, going back to sleep.

Contributor

envygeeks commented Jun 6, 2016

Hulk here,

I'm watching for you and I've restarted a test that randomly failed @DirtyF. @envygeeks So far no cases have replicated what you had me watching for. This is ready @envygeeks, it seems you were wrong, thanks for waking me up so early, going back to sleep.

@envygeeks envygeeks changed the title from Rubocop cleanup for misc files to Cleanup and make misc files compliant with Rubocop. Jun 6, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 6, 2016

Contributor

@jekyllbot: merge +dev

Contributor

envygeeks commented Jun 6, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit c8606c5 into jekyll:master Jun 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Jun 6, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 6, 2016

Contributor

❤️

Contributor

envygeeks commented Jun 6, 2016

❤️

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jun 6, 2016

Member

@envygeeks thanks a lot for your help. I learn some useful stuff along the way.

Member

DirtyF commented Jun 6, 2016

@envygeeks thanks a lot for your help. I learn some useful stuff along the way.

@parkr parkr referenced this pull request Jun 15, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment