Skip to content

Conversation

@vilyapilya
Copy link
Contributor

Adds internal logger instead of "puts"
Adds tests for all methods and unsuccessful request.

Rakefile Outdated
RSpec::Core::RakeTask.new(:spec)

task :default => :spec
task default: :spec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed by Rubocop

MS_IN_A_DAY = 86400000
MAX_REQUEST_TIMEOUT = 300000
MAX_LINE_LENGTH = 32000
LOG_LEVELS = %w[DEBUG INFO WARN ERROR FATAL TRACE].freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed by rubocop. I am not sure which one reads better

end

# Should retry to connect and preserve the failed line
def fatal_method_not_found(level, port, expected_level)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided not pass the parameter for repetition number (how many times to call the method and with which values for each time) bc I think it will be less readable.

Copy link
Contributor

@dchai76 dchai76 left a comment

Choose a reason for hiding this comment

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

Ugh, the rubocop-initiated changes make this harder to review. They should arguably have been in a separate PR, but whatever.

One request - it's nicer for your reviewers if you avoid all the temp commits with meaningless titles and squash them with a descriptive summary before you push your changes to remote. It just makes things easier to follow.

Copy link
Contributor

@dchai76 dchai76 left a comment

Choose a reason for hiding this comment

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

Let's leave the CircleCI changes for a separate PR - otherwise looks good.

Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

make sure to rebase with master after #13 gets merged in and make required correction to some files

@smusali
Copy link
Contributor

smusali commented Dec 20, 2019

make sure to rebase with master after #13 gets merged in and make required correction to some files

Besides rebasing, there are 2 things that RuboCop complained and couldn't fix should be handled:

  • .rubocop.yml: Metrics/LineLength has the wrong namespace - should be Layout
  • lib/logdna.rb:53:5: W: Lint/DuplicateMethods: Method Logdna::Ruby#level= is defined at both lib/logdna.rb:18 and lib/logdna.rb:53. def level=(value)

@vilyapilya vilyapilya merged commit f1c7757 into master Dec 20, 2019
@jakedipity jakedipity deleted the add-internal-logger branch January 25, 2021 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants