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

Feature/print struct like objects #226

Merged
merged 4 commits into from
May 12, 2016
Merged

Feature/print struct like objects #226

merged 4 commits into from
May 12, 2016

Conversation

waldyr
Copy link
Contributor

@waldyr waldyr commented Apr 15, 2016

@gerrywastaken
Copy link
Contributor

Thank you for the submission and sorry about the long delay in response. The failures in our Travis builds on master have now been fixed (git rebase upstream/master to update your commits) and I'm trying to go through outstanding PRs when I get time.

I tried rebasing this on master using my fork, but ended up with errors in the build: https://travis-ci.org/gerrywastaken/awesome_print/builds/129044884

@waldyr
Copy link
Contributor Author

waldyr commented May 11, 2016

Thank you for the submission and sorry about the long delay in response.

No worries at all. Time is an issue for everyone 😄

[...] I'm trying to go through outstanding PRs when I get time.

I'm really glad that you liked it even though I think that there should be a better solution. In that case I want to apologize for adding more cyclomatic complexity (that one conditional to check if it's an ivar or method). Maybe you can come up with something more elegant and I would be glad if you let me know about that.

I wish I could decouple some of the logic from the formatter's methods into specific classes for each handled type but as I said time is an issue for everyone.

result += @inspector.awesome(o.send(var))
end

key << result
end
Copy link
Contributor

@gerrywastaken gerrywastaken May 11, 2016

Choose a reason for hiding this comment

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

How do you feel about the following:

  • Changing the if condition to a call, to make it a bit more descriptive and pulling the logic outside of this method?
  • We could also write result += @inspector.awesome( after the branching to save writing it twice.
  • Adding a comment to the send line to explain it's inclusion.

e.g.

private

def valid_instance_var?(variable_name)
  variable_name.to_s.start_with?('@')
end

# [...]

  indented do
    var_contents = if valid_instance_var?(var)
      o.instance_variable_get(var)
    else
      o.send(var) # Enables handling of Struct attributes
    end

    key << colorize(" = ", :hash) + @inspector.awesome(var_contents)
  end

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's way better than before. Tonight I'll patch it up.

@gerrywastaken
Copy link
Contributor

gerrywastaken commented May 11, 2016

In that case I want to apologize for adding more cyclomatic complexity (that one conditional to check if it's an ivar or method).

That method was already doing way too much, it needs a complete overhaul.

I wish I could decouple some of the logic from the formatter's methods into specific classes

Agreed, that would be good in the long term.

@waldyr
Copy link
Contributor Author

waldyr commented May 12, 2016

Now that I can rely on the CI, next PR I'm gonna work on decoupling those methods into new classes. What do you think?

@gerrywastaken
Copy link
Contributor

Sounds fantastic! Although pease break it up into logical commits, like you have done with this one. :) That way it will be easy for me and everybody else to follow the changes.

Thanks heaps for this PR. 👍 💯

This will be in the next release, however I'm still waiting on access atm: https://github.com/michaeldv/awesome_print/issues/234

@gerrywastaken gerrywastaken merged commit 9dcd4a3 into awesome-print:master May 12, 2016
@gerrywastaken
Copy link
Contributor

This change went like in version 1.7.0. 👍

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.

2 participants