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

options classname and object_id #296

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aars
Copy link
Contributor

@aars aars commented Jan 10, 2017

Adds options to customize object classname and object_id output.

What do you think about this?

I don't think this is useful for many people, and it's probably better to introduce a way to add custom formatters (there isn't any at the moment, right?)

If it does seem useful, I think the new non-boolean/int option classname is probably a bad idea. A simple use_to_s switch would probably do, no?

For me personally, this combined with sort_vars options gives me great control over the output. Let me know what you think.

Cheers.

@gerrywastaken
Copy link
Contributor

gerrywastaken commented Feb 11, 2017

@aars There was previously talk about adding custom formatters but I'm not sure how far anybody got.

I agree with you that this change seems very niche. Perhaps if you explained the use case for changing the name of the class that is printed I might understand better if this will be useful for others.

@aars
Copy link
Contributor Author

aars commented Feb 13, 2017

Customer formatters would supersede this.

This is some actual (but contrived) test output I have.
pp some_nested_objects

#<SureBase::Expression::Operator:0x007fb574dd3138
    @op="divide",
    @input=0,
    @output=nil,
    @args=
     [#<SureBase::Expression::Operator:0x007fb574dd28a0
       @op="divide",
       @input=0,
       @output=nil,
       @args=
        [#<SureBase::Expression::Setter:0x007fb574dd2468
          @output=nil,
          @getset_data=nil,
          @args=
           [#<SureBase::Expression::Operator:0x007fb574dd2058
             @op="add",
             @input=0,
             @output=nil,
             @args=
              [#<SureBase::Expression::Value:0x007fb574dd1bf8 @expr=12, @output=nil>,
               #<SureBase::Expression::Value:0x007fb574dd1810 @expr=10, @output=nil>]>]>,
         #<SureBase::Expression::Value:0x007fb574dd1518 @expr=11, @output=nil>]>]>,

The long classnames and object_id's aren't very important in this "view". The structure of the data and the instance variables are. So I though, by giving the user control over that line as well, readability could be improved if necessary. And so, I did this:

ap some_nested_objects

[2] #<Operator::divide
      @input = 0,
      @output = nil,
      attr_reader :args = [
        [0] #<Operator::divide
          @input = 0,
          @output = nil,
          attr_reader :args = [
            [0] #<Setter['some_value']
              @output = nil,
              @getset_data = nil,
              attr_reader :args = [
                [0] #<Operator::add
                  @input = 0,
                  @output = nil,
                  attr_reader :args = [
                    [0] #<Value[12]
                      @expr = 12,
                      @output = nil
                    >,
                    [1] #<Value[10]
                      @expr = 10,
                      @output = nil
                    >
                  ]
                >
              ]
            >,
            [1] #<Value[11]
              @expr = 11,
              @output = nil
            >
          ]
        >
      ]
    >,

Here I removed :@op from the instance_variables and just added it to the classname string. I just won a line!

But I can totally see how this feature would be not only much better suited as a custom formatter, but might even invite more of these somewhat trivial, not-that-useful-too-most-people feature-PRs, and would clutter up the AP options.

(the inconsistency in use of [] or :: in the classname strings is simply my own inconsistent implementation)

@aars
Copy link
Contributor Author

aars commented Feb 13, 2017

If I want to look into writing custom formatter support, any tips on where to start? Any work been done? Or, any other libs out there with good custom formatter implementations we could copy or be inspired by? Logger maybe?

@aars
Copy link
Contributor Author

aars commented Feb 13, 2017

PS: I made a booboo. Fixed it, waiting for the CI.

@matthewhively
Copy link

While a custom formatter would be ideal in the mean time if there were simply a way to turn off the object_id (id) within the output of an object that would be fantastic.
My use case is I would like to easily compare the awesomely printed output from 2 different code versions using a simple command line diff. But since the object_ids always change it will always be flagged as a diff.

@aars
Copy link
Contributor Author

aars commented May 12, 2017

@matthewhively I'd suggest getting my changes to object_formatter from this PR. It does precisely that.

I don't know what's up with the CI in here. But I would agree that custom formatters would be a better feature, so I don't mind this not getting merged. Though I am currently running my own version of AP with this PR included.

@matthewhively
Copy link

@aars Yes, I see that this PR does indeed provide the functionality I want. That is exactly why I replied to it. I wanted to provide another use case in the hope that this might get merged.

@imajes
Copy link
Collaborator

imajes commented Jun 16, 2017

hey @aars, this has been a million years ignored (sorry!) but i see the value in this -- and since we've not been good at updating ap, i figured work done but not perfect > nothing happening. I'd love this if you could rebase on the conflict, if not i'll try to get to it soon! thx.

@imajes imajes closed this Jun 16, 2017
@imajes imajes reopened this Jun 16, 2017
@aars
Copy link
Contributor Author

aars commented Jun 20, 2017

I think I can manage that yes. Sometime this week.

@imajes
Copy link
Collaborator

imajes commented Nov 7, 2017

@aars: hey! just wanted to ping ya on this. It'd be great to get this rebase done, and get this work merged into the next release. I like this pr, and would love to see your hard work land!

thanks. 👍 🥇

@aars
Copy link
Contributor Author

aars commented Nov 7, 2017

Ugh! Sorry. Hectic weeks (uh.. yeah.. since June... Ok... not an excuse... :D )

I've got some more github related stuff to do this week. I've put this on my todo list.

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.

4 participants