Skip to content

feat(gem): add option to omit private methods - #2743

Open
efegokdemir wants to merge 2 commits into
Shopify:mainfrom
efegokdemir:efe/tapioca-omit-private-gem-methods
Open

efegokdemir wants to merge 2 commits into
Shopify:mainfrom
efegokdemir:efe/tapioca-omit-private-gem-methods

Conversation

@efegokdemir

Copy link
Copy Markdown

Motivation

Closes #2740. Including every private method can make generated gem RBIs unnecessarily large.

Implementation

Add gem.private_methods (default true) and gem.private_methods_for (default []). The filter runs after exported RBIs are merged, so private methods are omitted from the final RBI while selected gems can retain them. The options are documented in the README and generated config comments.

Tests

The targeted private-method and configuration tests passed, as did bin/typecheck, bin/style on the changed Ruby files, and bin/readme. The broader gem/config specs encounter an existing Rails Git fixture cache error; the same Rails fixture failure was reproduced on pristine main.

@dduugg dduugg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for picking this up! The option shape and the placement after merge_with_exported_rbi match what the issue proposed.

remove_private_methods skips every other adjacent private method

RBI::Node#detach calls tree.nodes.delete(self), so detaching inside tree.nodes.each shifts the next node into the current index, and each never visits it. Since private methods are emitted next to each other, roughly half of them survive. A gem with

module Foo
  def public_method; end

  private

  def priv_a; end
  def priv_b; end
  def priv_c; end
  def priv_d; end

  class << self
    private

    def cpriv_a; end
    def cpriv_b; end
  end
end

and private_methods: false generates:

module Foo
  def public_method; end

  private

  def priv_b; end
  def priv_d; end

  class << self
    private

    def cpriv_b; end
  end
end

The new spec passes because each private method in its fixture is followed by a public one. Iterating over a copy fixes it, which is what RBI::Rewriters::FlattenVisibilities does:

tree.nodes.dup.each do |node|

I tried the branch on the app from #2740, with the eight public gems the issue lists in private_methods_for. Of the 21,553 private methods in the other gems, 9,121 were still in the generated RBIs. With .dup, none were left, and srb tc passed after I also added spoom and erubi (Spoom::Deadcode::ERB overrides private methods of Erubi::Engine).

Specs

The repo's gem specs usually compare the whole file with assert_project_file_equal and a heredoc. That would have caught the leftover methods, where assert_includes/refute_match on single methods did not. I'd suggest one spec that runs tapioca gem foo three times (default config, private_methods: false, then with private_methods_for: [foo]) and asserts the full RBI each time, using a fixture with adjacent private instance methods and a private singleton method. That also covers what the separate "must include private methods by default" spec checks.

Not from this PR, FYI

Methods declared only in an exported RBI under a bare private section still come out public, so the filter can't see them. The RBI parser records bare private as a standalone RBI::Private node and leaves the following methods public, and the merge drops that node. main already behaves this way, so it doesn't need to block this PR. I opened #2751 to flatten the exported visibilities before merging.

This branch has not been deployed

No deployments
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.

Option to omit private methods from gem RBIs, with a per-gem opt-in

2 participants