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

Alignment of method arguments? #83

Closed
ivoanjo opened this issue Mar 3, 2018 · 3 comments · Fixed by #215
Closed

Alignment of method arguments? #83

ivoanjo opened this issue Mar 3, 2018 · 3 comments · Fixed by #215

Comments

@ivoanjo
Copy link

ivoanjo commented Mar 3, 2018

Hello there! First of all: thank you very much for Rufo! I hope it takes off like a Falcon rocket 🚀

I was playing with it on one for my personal projects, and noticed that unlike the examples listed in the aligning call parameters section of the README, the alignment on method arguments is not preserved.

Example:

Input file:

def method_with_a_lot_of_args(
  hello:,
  world:,
  foo:,
  bar:,
  baz:
)
  puts "hello, world!"
end

method_with_a_lot_of_args(
  hello: 1,
  world: 2,
  foo: 3,
  bar: 4,
  baz: 5
)

Rufo output:

def method_with_a_lot_of_args(
                              hello:,
                              world:,
                              foo:,
                              bar:,
                              baz:)
  puts "hello, world!"
end

method_with_a_lot_of_args(
  hello: 1,
  world: 2,
  foo: 3,
  bar: 4,
  baz: 5,
)

My expectation: I would like for Rufo to also keep the existing alignment, as it does on method calls.

What do you think? I know that Rufo is not about personal opinion, but this result was quite unexpected to me, as I wouldn't expect different rules for calling a method from declaring a method.

Thanks! 🙏

@gingermusketeer
Copy link
Member

Thanks for reporting this. Do you happen to know how this is commonly handled in the community?

I personally would do what you have done and suspect that would be the more common approach, but be good to get some data.

@ivoanjo
Copy link
Author

ivoanjo commented Mar 4, 2018

Excellent question! Time for some research:

By download popularity

Looking at the top gems by rubygems download stats here is what I saw (in rough order of popularity):

bundler

Never breaks arguments into multiple lines. Only examples broken into multiple lines I found in their repository come from vendored gems.

Sample methods with longest method names + argument lists:

  • def self.resolve(requirements, index, source_requirements = {}, base = [], gem_version_promoter = GemVersionPromoter.new, additional_base_requirements = [], platforms = nil) link
  • def initialize(env, output_file, show_version = false, show_requirements = false, output_format = "png", without = []) link

rspec-core

Only found a single example of breaking arguments into multiple lines:

        def print_example_failed(pending_fixed, description, run_time, failure_id,
                                 exception, extra_content)

Other than that, the widest lines are still not very wide so they aren't broken (or so I assume):

  • def initialize(example, exception_presenter=Formatters::ExceptionPresenter::Factory.new(example).build) link

  • def find_minimal_repro(output, formatter=Formatters::BisectProgressFormatter, bisect_runner = :fork) link

rspec-mocks

Like rspec-core, there are very few examples of breaking into multiple lines, and they follow the same style:

      def raise_expectation_error(message, expected_received_count, argument_list_matcher,
                                  actual_received_count, expectation_count_type, args,
                                  backtrace_line=nil, source_id=nil)
        def initialize(error_generator, expectation_ordering, expected_from, method_double,
                       type=:expectation, opts={}, &implementation_block)

Other wide-ish lines are not broken (below 120 chars).

rails gems: activesupport, actionpack, actionview, activemodel, activerecord, actionmailer, activejob, actioncable, activestorage, railties

Developers almost never break lines (even long ones).

Longest unbroken lines:

  • def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, expires_in: nil, race_condition_ttl: nil, error_handler: DEFAULT_ERROR_HANDLER, **redis_options) link

  • def initialize(object_name, method_name, template_object, collection, group_method, group_label_method, option_key_method, option_value_method, options, html_options) link

Broken lines show a mix of styles:

  • (activerecord) link
      def initialize(
        table, name,
        unique = false,
        columns = [],
        lengths: {},
        orders: {},
        opclasses: {},
        where: nil,
        type: nil,
        using: nil,
        comment: nil
      )

(There's two such examples on this file)

  • (activerecord) link
      def define_attribute(
        name,
        cast_type,
        default: NO_DEFAULT_PROVIDED,
        user_provided_default: true
      )
          def initialize(template_object, object_name, method_name, object,
                         sanitized_attribute_name, text, value, input_html_options)
        def html_options_for_form_with(url_for_options = nil, model = nil, html: {}, local: !form_with_generates_remote_forms,
          skip_enforcing_utf8: nil, **options)

tzinfo

Developers almost never break lines (even long ones).

Only two broken lines I found:

      def timezone(identifier_or_reference, latitude_numerator = nil,
                  latitude_denominator = nil, longitude_numerator = nil,
                  longitude_denominator = nil, description = nil)
      def timezone(reference, identifier, latitude_numerator, latitude_denominator,
                    longitude_numerator, longitude_denominator, description = nil)

nokogiri

No methods with long enough method + argument names + initializations to warrant breaking lines... I think. They actually do not use brackets for defs so it's harder to check for multi-line defines.

sass

Most lines are not long enough to need breaking, but the few that are use

      def populate_extends(extends, extendee, extend_node = nil, parent_directives = [],
          allow_compound_target = false)
    def join(list1, list2,
             separator = identifier("auto"), bracketed = identifier("auto"),
             kwargs = nil, *rest)

multi_json, rake, rack, json, diff-lcs, rspec-expectations, mime-types, i18n, thor, rspec-support, builder, tilt, minitest, arel, erubis, faraday, execjs, pry, sinatra gems

No methods with long enough method + argument names + initializations to warrant breaking lines.


Newer gems

Because counting by download favors gems that have been around for a long time, let's look at some newer gems:

puma

Most lines are not long enough to need breaking, but the single one that is:

    def add_ssl_listener(host, port, ctx,
                         optimize_for_latency=true, backlog=1024)

trailblazer, roda, concurrent-ruby, dry-configurable, dry-container, dry-equalizer, dry-types gems

No methods with long enough method + argument names + initializations to warrant breaking lines.


Style guides

Ruby community style guide

No guidelines for this.


My observations

It's great that the community avoids having too many arguments on their methods. I bet I wouldn't get the same results from Java code ;)

In general, it seems the community prefers not to break longer lines, even when they start getting really long.

When they do, they rarely separate each argument on its own line. In these cases, they almost always align to the opening bracket.

I found a few examples on the rails repository of having each argument in its own line, and these use the style I suggested rufo adopts.

I was not able to find in any of the gems I searched code that matched the current style rufo employs of aligning to opening bracket when using one argument per line.

Finally, just to make this discussion a bit less abstract, here is one example of the way I used this in my own code:

link

    def initialize(
      requester_factory: Requester,
      wait_for_port_factory: WaitForPort,
      logger: WarmBlanket.config.logger,
      endpoints: WarmBlanket.config.endpoints,
      hostname: 'localhost',
      port: WarmBlanket.config.port,
      warmup_threads: WarmBlanket.config.warmup_threads,
      warmup_time_seconds: WarmBlanket.config.warmup_time_seconds
)

I find that this style is especially useful when there are defaults for arguments, as otherwise it would be hard to avoid longer lines in methods that have longer names.

@gingermusketeer
Copy link
Member

@ivoanjo Wow that is amazing, thanks for the solid research.

I think I agree with your suggestion. The rationale being:

  1. As you have shown not many codebase would be drastically affected.
  2. The new formatter will hopefully apply the same logic for arrays, hashes, method calls and method definitions.

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 a pull request may close this issue.

2 participants