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

DRY - Code Refactor #310

Closed
wants to merge 1 commit into from

Conversation

manjunath724
Copy link

  • case..when..else..end blocks have been aligned vertically
  • Moved common code snippets to a private methods
  • Used eval(str) to return Boolean true/false
  • Removed !!(Double not) prepended to true/false

@manjunath724
Copy link
Author

While working PR #309. I noticed that a lot of code could be refactored - which has led to this PR #310. Can we please approve the tests to run on this branch?

)
end

def instance_of(value)
Copy link
Member

@Fryguy Fryguy Jan 12, 2022

Choose a reason for hiding this comment

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

instance_of is a strange name here, but I'm struggling coming up with a better name. To me, instance_of implies you are getting another instance of something (perhaps a dup), but it's actually returning the converted value of the passed in value. Perhaps

Suggested change
def instance_of(value)
def serialize_value(value)

Once that is changed, I was thinking you can also rename descend_array to serialize_array to match the pattern, but now that I look at it, there's no longer a reason to have a separate method for handling an array, and it can probably be inlined directly into this method.

@@ -9,7 +9,7 @@ class YAMLSource

def initialize(path, evaluate_erb: Config.evaluate_erb_in_yaml)
@path = path.to_s
@evaluate_erb = !!evaluate_erb
@evaluate_erb = evaluate_erb
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this one...it's kind of nice having a proper boolean, since it is accessible via the attr_reader.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the same page as you @Fryguy . Several of the changes proposed in this PR compromise correctness or readability in favor of succinctness.

I'm comfortable with the serialize_value suggestion as a separate method as well as the formatting for case/when but I'm not excited about the other changes.

@cjlarose
Copy link
Member

cjlarose commented Mar 2, 2024

Closing because this is stale

@cjlarose cjlarose closed this Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants