I’m confused. The page already groups them by class methods and instance methods. Can’t this PR be accomplished simply by adding two h3s? What’s the purpose of the massive overhaul? I don’t know about how Colby feels and it’s his project to manage, but I personally try to be mindful of whether an importer change is necessary, since it requires a reimport of all Ruby versions in order to deploy the code.

The page already groups them by class methods and instance methods.

No, only sidebar.

Can’t this PR be accomplished simply by adding two h3s?

No. Do you see that there was one cycle and there is now two separated?

OK, from master version:

Do you see this? File.file?, then file.flock and then File.fnmatch, so class-instance-class methods.

No, only sidebar.

Ahh, that's what I was remembering.

I need this for other ideas (PRs). Can we merge it? Should I do something else?

Can I help you somehow?

Apologies for the really long delay in responding. I've had lots of life stuff happening.

I've had a quick look at the PR and I agree with separating out the instance & class methods in the UI, but I don't agree wth separating them in ElasticSearch. I don't think having 2 sets of methods improves the overall abstraction, but I am happy to add some helper methods into the model/views to cleanup some of the logic around handling them.

Apologies for the really long delay in responding. I've had lots of life stuff happening.

No problems. 🤝

but I don't agree wth separating them in ElasticSearch. I don't think having 2 sets of methods improves the overall abstraction

Why? It's like memory vs CPU. I think we can waste more memory for this, but gain performance without many many methods.select. I think CPU is more important (more expensive).

@colby-swandale sorry, but I'm waiting for answers (some explanation). And I want to work further on this awesome product.

Any improvements to performance is great but it's not something I want prioritised over everything else. Most pages are being served under 200ms and is being cached in Cloudflare for everyone.

I had a second look at the PR and my feelings are the same as in my comment. I definitely like the separation of instance & class methods in the UI, but I would prefer maintaining just a single source of methods for RubyObject in ElasticSearch and have public API to expose the list of class & instance methods (you can even do caching there).

Thanks.

I had a second look at the PR and my feelings are the same as in my comment. I definitely like the separation of instance & class methods in the UI, but I would prefer maintaining just a single source of methods for RubyObject in ElasticSearch and have public API to expose the list of class & instance methods (you can even do caching there).

OK, I'll rewrite it, thanks. Then we will able to compare these approaches by code, at least just as interesting information. 🙃

I had a second look at the PR and my feelings are the same as in my comment. I definitely like the separation of instance & class methods in the UI, but I would prefer maintaining just a single source of methods for RubyObject in ElasticSearch and have public API to expose the list of class & instance methods (you can even do caching there).

Done. Can you check, please?

Then we will able to compare these approaches by code, at least just as interesting information.

There is more code BTW, if somebody is interested in.

Given what Colby said before, I wouldn’t expect to see any importer changes in this PR. What’s the purpose of the importer changes you’ve made?

Given what Colby said before, I wouldn’t expect to see any importer changes in this PR. What’s the purpose of the importer changes you’ve made?

To clarify: this is about these changes: a5dc18a#diff-4f156e6b03325d00e2526d01f21339ef

There are only small optimizations, as code-style (RuboCop asks an empty line after guard-clause like next), as well CPU (we're used method_doc.name twice). Also, I've changed the values of Method#method_type from *_method (class_method) to a simple * (class). I see a big tautology there: Method method type type method. So, I've decided to cut it. Honestly, I'd like Method#type # => 'class'.

I really feel like you should split this into multiple PRs. There's 12 files changed, when it feels like only like 4 or 5 are relevant to the purpose of "Split class and instance methods in model".

PRs I see:

1. Split class and instance methods in model
2. Remove "_method" from method_type property
3. Performance improvement to RDoc Generator method_name

1 is obviously this PR (very valuable feature).

2 is very debatable, considering Colby intentionally added it in the first place. I think you and him will require a decent amount of discussion to get this one approved. This also means that all 5 supported Ruby versions will need to be reimported with this code change (a step that's not necessary for a simple HTML change).

3 is also debatable, since it's already memoized into an instance variable internally, so you're just caching the results of an instance variable read into a local variable. The performance improvement is pretty negligible, and not super important since it's just the importer. Seeing this change made me wonder if I was missing something -- like maybe you were changing what the method name is (or how it's determined).

https://github.com/ruby/rdoc/blob/0ead78616bbddfbdc4d21a3776df16abc25ab811/lib/rdoc/any_method.rb#L223-L230

require 'benchmark/ips'

Benchmark.ips do |x|
  @test_read_var = 1

  def test_read
    @test_read_var
  end

  cached_test_read_var = test_read

  x.report('local variable') do
    cached_test_read_var
  end

  x.report('method call') do
    test_read
  end

  x.compare!
end

Warming up --------------------------------------
      local variable     2.739M i/100ms
         method call     2.043M i/100ms
Calculating -------------------------------------
      local variable     27.365M (± 2.1%) i/s -    136.964M in   5.007354s
         method call     20.055M (± 3.6%) i/s -    102.161M in   5.101165s

Comparison:
      local variable: 27364900.7 i/s
         method call: 20055392.7 i/s - 1.36x  (± 0.00) slower

My point in saying this is, you have a really good feature add here, but I feel like the PR is bloated with other unrelated things. And with 12 files to review, it can feel a little overwhelming. It also left me wondering why certain changes were necessary, and it seems like the answer is that they aren't really necessary to this specific feature, but are things you think would be good to change. I personally think it'd be easier to read through multiple smaller PRs. Additionally, Colby is doing this for free, and may not have much time he can devote to this right now (a common problem for open source maintainers). Multiple small PRs means maybe he can review a small PR per day and get everything out, instead of struggling to get through one large PR.

I hope this comment was helpful.

I really feel like you should split this into multiple PRs.

I like the approach when you're doing something and improve something related. But OK, I can agree with you here and I'll split.

Thank you for detailed answer.

BTW, your benchmark is not fully correct, and I don't want to bloat the discussion of this PR, and I'll check it more correctly and describe if I'll find a significant improvement. But I didn't see that there is an instance caching, so I think it will not.

Conflicts resolved.

This looks almost ready to go! I had a second look at the UI and found it a tiny bit difficult to read the header for class/instance sections amongst each of the method headers. I tweaked a couple of things around to help clear it up in 6d61413. Let me know what you think.

I had a second look at the UI and found it a tiny bit difficult to read the header for class/instance sections amongst each of the method headers. I tweaked a couple of things around to help clear it up in 6d61413. Let me know what you think.

I don't like it. Why it's not bold or semi-bold? The strange header between regular bold headers. I had no problems to read these headers and distinguish them from neighboring ones.

Also, I see you've added conditions @object.ruby_instance_methods.empty?. It's important and not related to the commit message. Also, I think it should be in the more higher level, to not render hr, for example.

When I first looked at this PR a few weeks back, I remember feeling like it was a little hard for me to find the "Instance Methods" title. My first inclination was that instead of shrinking other text so that "Integer" could be the same size, maybe making that bigger would give a little more room for "Class Methods" and "Instance Methods" to be larger. I also felt like some space between the class methods list and the word "Instance Methods" would help me find that next section easier.

I haven't side by side compared Alex's UI to Colby's, so I can't comment on that. But I did want to echo the sentiment that I felt it needed a little tweaking to be easier to find that next section amongst all the content of the class methods.

I haven't side by side compared Alex's UI to Colby's, so I can't comment on that.

There are screenshots of both versions.

I will try to tweak these headers, OK.

@AlexWayfer your screenshot doesn't include the "Instance Methods" header next to a bunch of class methods. This is what I remember having a harder time finding when I last ran the code. I haven't had time to run it lately, but was trying to provide some feedback, since that's what the conversation is about.

@AlexWayfer your screenshot doesn't include the "Instance Methods" header next to a bunch of class methods.

That's true.

Here is a new version:

I think, larger margin-top with hr make it good enough.

What do you think?

Also I notice that I've replaced some base padding styles with margin ones because margin is more native (especially for <p>) and works better with its overlapping.

Conflicts have been resolved again.

Apologies but I'm still having a really hard time trying to distinguish between the method name header & the "Instance method" & "Class method" headers, they almost look like the the same thing in this screenshot.

I don't want this PR to end up being in limbo going back and forth between trying to find the perfect header, so I'm tempted to just merge this now and work on the headers in another PR.

I don't want this PR to end up being in limbo going back and forth between trying to find the perfect header, so I'm tempted to just merge this now and work on the headers in another PR.

Good point. Sorry for rejecting your code suggestion, I appreciate it, but I don't like too significant deviation from standard styles, like normal font weight for a header between bold font weight of two headers (above and below). I hope you're understanding me.

Conflicts resolved (again), discussions too.

@colby-swandale I definitely think something needs to change with the headers.

I'm glad this got merged, because I think the styling can be sorted out easier in a separate PR.

My critique is that on desktop at least, the "Class Methods" and "Instance Methods" title is the same size as the actual method name (or call sequence). It's all 24px (lg:text-2xl) and then the constant name at the top (eg "String") is 30px (lg:text-3xl). This makes it hard for me to find the "Instance Methods" section, and also doesn't seem to set apart the sections from the methods enough. In the standard h1, h2, h3 world, I'd expect "String" to be biggest, "Class Methods" to be second biggest, the "String.new" call sequence to be third biggest, then of course the method description is the smallest of the 4 things.

Perhaps a solution is to do lg:text-xl (18px) for the call sequence?

Perhaps a solution is to do lg:text-xl (18px) for the call sequence?

Good idea. I don't know why I didn't think about it. Looks good for me, I've created #534.