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

Cleanup to match best practices in ruby and shell #98

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

teancom
Copy link

@teancom teancom commented Oct 23, 2018

Cleanup both ruby and shell scripts to match various linters'
suggestions. Mostly using the new hash syntax, as well as small
readability changes. Additionally, update gitignore to not accidentally
add in the output from running rake vendor

This was done as a "slate-cleaning" and a way to learn the repo before starting any of the issues marked "hacktober".

@teancom
Copy link
Author

teancom commented Oct 23, 2018

The CI build failed but it appears like it was due to connectivity issues. Is that something that just needs to be kicked off again?


# The host or ip to bind
config :host, :validate => :string, :default => "0.0.0.0"
config :host, validate: :string, default: '0.0.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

we tend to stick to => notation for hashes in the logstash and logstash plugins codebase

@jsvd
Copy link
Member

jsvd commented Oct 23, 2018

Thanks for the PR @teancom, I have restarted the tests in travis

@@ -243,19 +238,19 @@ def build_ssl_params
end

def ssl_key_configured?
!!(@ssl_certificate && @ssl_key)
!(@ssl_certificate && @ssl_key).nil?
Copy link
Member

Choose a reason for hiding this comment

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

let's skip these changes, IMO it's debatable which one of !!expr or !(expr).nil? is better

end

def encoding_handled?(env)
['gzip', 'deflate'].include? env['HTTP_CONTENT_ENCODING']
%w[gzip deflate].include? env['HTTP_CONTENT_ENCODING']
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather stick with an array here, personal preference

source ./ci/setup.sh

if [[ -f "ci/run.sh" ]]; then
echo "Running custom build script in: `pwd`/ci/run.sh"
echo "Running custom build script in: $(pwd)/ci/run.sh"
Copy link
Member

Choose a reason for hiding this comment

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

if we're indenting these lines we should indent the remaining ones in the block

Cleanup both ruby and shell scripts to match various linters'
suggestions. Mostly using the new hash syntax, as well as small
readability changes. Additionally, update gitignore to not accidentally
add in the output from running `rake vendor`
@teancom
Copy link
Author

teancom commented Oct 23, 2018

I reverted/cleaned up the issues you found, squashed, and repushed. Let me know if it looks better now. Thank you for taking the time to review!

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.

3 participants