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

Minor refactoring #61

Merged
merged 6 commits into from Jul 5, 2020
Merged

Minor refactoring #61

merged 6 commits into from Jul 5, 2020

Conversation

utkarsh2102
Copy link
Contributor

Hi @arnau 👋

Here's my take on doing some minor refactoring and most importantly, getting the CI go green again! 🎉

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
.travis.yml Outdated Show resolved Hide resolved
Copy link
Owner

@arnau arnau left a comment

Choose a reason for hiding this comment

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

I ran rake and RuboCop seems to be complaining:

Running RuboCop...
Inspecting 32 files
............C...................

Offenses:

lib/iso8601/months.rb:66:5: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for calculation is too high. [7/6]
    def calculation(atom, base) ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^

32 files inspected, 1 offense detected
RuboCop failed!

@utkarsh2102
Copy link
Contributor Author

I ran rake and RuboCop seems to be complaining

What version of RuboCop d'you have?

@arnau
Copy link
Owner

arnau commented Jul 5, 2020

I ran rake and RuboCop seems to be complaining

What version of RuboCop d'you have?

# cat Gemfile.lock
  38   │     rubocop (0.85.1)
  39   │       parallel (~> 1.10)
  40   │       parser (>= 2.7.0.1)
  41   │       rainbow (>= 2.2.2, < 4.0)
  42   │       regexp_parser (>= 1.7)
  43   │       rexml
  44   │       rubocop-ast (>= 0.0.3)
  45   │       ruby-progressbar (~> 1.7)
  46   │       unicode-display_width (>= 1.4.0, < 2.0)

@utkarsh2102
Copy link
Contributor Author

Okay, I'll put this in .rubocop.yml so this shouldn't be a blocker.

@arnau
Copy link
Owner

arnau commented Jul 5, 2020

I use nix to isolate the environment and I had to add rake to it. Would you mind adding it to this PR?

# cat default.nix
let
  pkgs = import <nixpkgs> {};
  tooling = [
    pkgs.exa
    pkgs.xsv
    pkgs.git
    pkgs.ruby_2_7
    pkgs.rubyPackages_2_7.rake
  ];

in
  pkgs.mkShell {
    LANG = "en_GB.UTF-8";
    EDITOR="vim";
    buildInputs = tooling ++ [
    ];

    shellHook = ''
      set -o vi
      local pink='\e[1;35m'
      local yellow='\e[1;33m'
      local blue='\e[1;36m'
      local white='\e[0;37m'
      local reset='\e[0m'

      function git_branch {
        git rev-parse --abbrev-ref HEAD 2>/dev/null
      }

      export PS1="\[$pink\]nix \[$blue\]\W \[$yellow\]\$(git_branch)\[$white\] ∙ \[$reset\]"

      alias ls=exa

      source ${pkgs.git}/share/bash-completion/completions/git
    '';
  }

@arnau
Copy link
Owner

arnau commented Jul 5, 2020

Although, perhaps rake should be a dependency in the gemspec?

It's there 🤦 but doesn't work for me for some reason.

@utkarsh2102
Copy link
Contributor Author

Okay, I'm going to experiment with a few changes and commits here. Shall squash the related commits in the end.
And then ping you at the end for a review.

Meanwhile if you've any idea about CI failure because of RubyTargetVersion, let me know 😪😅

@arnau
Copy link
Owner

arnau commented Jul 5, 2020

If I'm reading it correctly, the problem comes from rainbow:

Error: RuboCop found unsupported Ruby version 2.1 in TargetRubyVersion parameter (in vendor/bundle/ruby/2.7.0/gems/rainbow-3.0.0/.rubocop.yml). 2.1-compatible analysis was dropped after version 0.57

Which is a RuboCop dependency 🤔

@utkarsh2102
Copy link
Contributor Author

Weehoo, CI is green again!
https://travis-ci.org/github/arnau/ISO8601

The problem was that bundler was cached 😄

@arnau
Copy link
Owner

arnau commented Jul 5, 2020

What about this https://docs.rubocop.org/rubocop/configuration.html#setting-the-target-ruby-version ?

AllCops:
  TargetRubyVersion: 2.5.0

@utkarsh2102
Copy link
Contributor Author

So I am going to finally squash them and it should be ready in the next ~10 minutes.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@utkarsh2102
Copy link
Contributor Author

@arnau, green and ready from my side! 🎉

@arnau arnau merged commit 1ebbe8f into arnau:master Jul 5, 2020
@utkarsh2102 utkarsh2102 deleted the minor-refactoring branch July 5, 2020 08:27
@arnau
Copy link
Owner

arnau commented Jul 5, 2020

Unless you have other things in mind, I'll be releasing v0.13.0 soon.

@utkarsh2102
Copy link
Contributor Author

Sure, looks good for v0.13.0 🎉

@arnau
Copy link
Owner

arnau commented Jul 5, 2020

Fantastic! nice work, thanks :)

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.

None yet

2 participants