[ruby-mode] Private/protected method definition layout in Ruby 2.1

[ruby-mode] Private/protected method definition layout in Ruby 2.1

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Ruby 2.1 has made it possible to define private/protected/public methods on a per method basis (because a `def` now returns a symbol instead of nil). It means that now it’s possible to write code like this:

private def map(attribute, to:)
  @rules[attribute] = to
end


We currently don’t support such an indentation scheme, as `def` is always aligned with `end`:

private def map(attribute, to:)
              @rules[attribute] = to
            end


I’m not saying the first alignment is preferable, but it light of recent changes we did to accommodate similar alignment for `if/unless` it seems like a good idea to add a defcustom supporting the first style as well.

vim already added support for it (https://github.com/vim-ruby/vim-ruby/issues/190).

More on the topic can be read here http://www.sitepoint.com/look-ruby-2-1/.

--  
Cheers,
Bozhidar


[-- Attachment #2: Type: text/html, Size: 1794 bytes --]

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Stefan Monnier]

> Ruby 2.1 has made it possible to define private/protected/public
> methods on a per method basis (because a `def` now returns a symbol
> instead of nil). It means that now it’s possible to write code like
> this:
> private def map(attribute, to:)
>   @rules[attribute] = to
> end
> We currently don’t support such an indentation scheme, as `def` is
> always aligned with `end`:
> private def map(attribute, to:)
>               @rules[attribute] = to
>             end

I presume you meant

  private def map(attribute, to:)
            @rules[attribute] = to
          end

> I’m not saying the first alignment is preferable,

That's surprising.  The current alignment just looks like a plain bug to
me and can't think of any reason why someone would prefer it over

  private def map(attribute, to:)
    @rules[attribute] = to
  end

Of course, I don't know Ruby very well, so maybe this just shows my ignorance.

In any case, what I wanted to say here, is that in my experience, the
easiest way to handle such things with SMIE is to make the token code
treat "private def" as a single token (and to call it "def").  This may
sound silly, but trying to DTRT (for similar issues in other modes) by
tweaking grammar plus indentation rules has proved a lot more painful.


        Stefan

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I’m not saying the first alignment is preferable,
>
> That's surprising.  The current alignment just looks like a plain bug to
> me and can't think of any reason why someone would prefer it over

I also think it looks like a bug, but it's just indentation of an
arguments in a paren-less function call. `private' is a method, called
with the expression `def ... end`.

That said, I think indenting it to the beginning of `private' would be
preferable, as it's consistent with Vim and examples on the Web.

> In any case, what I wanted to say here, is that in my experience, the
> easiest way to handle such things with SMIE is to make the token code
> treat "private def" as a single token (and to call it "def").  This may
> sound silly, but trying to DTRT (for similar issues in other modes) by
> tweaking grammar plus indentation rules has proved a lot more painful.

I believe we can also handle this in the rules-function. Add a new rule
for (:before . "def"), see if the preceding token is one of `private',
`public', `protected', and if so, indent to its beginning column.

Can we install such a change, with either of the approaches, during the
feature freeze?

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Stefan Monnier]

> `private' is a method, called with the expression `def ... end`.

Ah, that makes sense.

> I believe we can also handle this in the rules-function.  Add a new rule
> for (:before . "def"), see if the preceding token is one of `private',
> `public', `protected', and if so, indent to its beginning column.

If that's works, it's preferable, indeed.  Especially in light of the
above (i.e. changing the parsing would be wrong).

> Can we install such a change, with either of the approaches, during the
> feature freeze?

I think it's OK, yes.  We're not yet in the "regression-only" phase.


        Stefan

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Dmitry Gutov]

Bozhidar Batsov <bozhidar@batsov.com> writes:

> recent changes we did to accommodate similar alignment for `if/unless
> ` it seems like a good idea to add a defcustom supporting the first
> style as well.

Come to think of it, why not reuse the same mechanism and defcustom?

The patch below seems to work. Comments?

=== modified file 'lisp/progmodes/ruby-mode.el'
--- lisp/progmodes/ruby-mode.el	2014-01-10 16:32:45 +0000
+++ lisp/progmodes/ruby-mode.el	2014-01-16 05:35:37 +0000
@@ -226,7 +226,10 @@
   :group 'ruby
   :safe 'integerp)
 
-(defcustom ruby-align-to-stmt-keywords nil
+(defconst ruby-alignable-keywords '(if while unless until begin case for def)
+  "Keywords that can be used in `ruby-align-to-stmt-keywords'.")
+
+(defcustom ruby-align-to-stmt-keywords '(def)
   "Keywords after which we align the expression body to statement.
 
 When nil, an expression that begins with one these keywords is
@@ -250,17 +253,13 @@
 
 Only has effect when `ruby-use-smie' is t.
 "
-  :type '(choice
+  :type `(choice
           (const :tag "None" nil)
           (const :tag "All" t)
           (repeat :tag "User defined"
-                  (choice (const if)
-                          (const while)
-                          (const unless)
-                          (const until)
-                          (const begin)
-                          (const case)
-                          (const for))))
+                  (choice ,@(mapcar
+                             (lambda (kw) (list 'const kw))
+                             ruby-alignable-keywords))))
   :group 'ruby
   :safe 'listp
   :version "24.4")
@@ -639,7 +638,7 @@
           (smie-indent--hanging-p)
           ruby-indent-level))
     (`(:after . ,(or "?" ":")) ruby-indent-level)
-    (`(:before . ,(or "if" "while" "unless" "until" "begin" "case" "for"))
+    (`(:before . ,(guard (memq (intern token) ruby-alignable-keywords)))
      (when (not (ruby--at-indentation-p))
        (if (ruby-smie--indent-to-stmt-p token)
            (ruby-smie--indent-to-stmt)

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

I'm OK with the patch, but it makes configuration a bit more difficult
since users should actually know all the alignable keywords. I guess we can
mention `ruby-alignable-keywords' in the docstring
of `ruby-align-to-stmt-keywords' and consider this a good enough hint for
the users. One problem with the current implementation is that it won't
play nice with assignments if you won't to treat them differently:

x = def something
  ala
  bala
end

There won't be a way to get this indentations at the same time:

private def something
  ala
  bala
end

and

x = def something
        ala
        bala
      end

I think that it might make sense to support a different def alignment for
`def` after `private`, `protected`, `public` regardless of the
`ruby-align-to-stmt-keywords' value. What I'm saying is that some people
might prefer to align `def` with a statement beginning only with access
modifier methods. Of course, it seems unlikely that someone will assign the
value of a method def to a variable, but in the future a method definition
in MRI, but it makes sense in Rubinius for instance.



On 16 January 2014 07:47, Dmitry Gutov <dgutov@yandex.ru> wrote:

> Bozhidar Batsov <bozhidar@batsov.com> writes:
>
> > recent changes we did to accommodate similar alignment for `if/unless
> > ` it seems like a good idea to add a defcustom supporting the first
> > style as well.
>
> Come to think of it, why not reuse the same mechanism and defcustom?
>
> The patch below seems to work. Comments?
>
> === modified file 'lisp/progmodes/ruby-mode.el'
> --- lisp/progmodes/ruby-mode.el 2014-01-10 16:32:45 +0000
> +++ lisp/progmodes/ruby-mode.el 2014-01-16 05:35:37 +0000
> @@ -226,7 +226,10 @@
>    :group 'ruby
>    :safe 'integerp)
>
> -(defcustom ruby-align-to-stmt-keywords nil
> +(defconst ruby-alignable-keywords '(if while unless until begin case for
> def)
> +  "Keywords that can be used in `ruby-align-to-stmt-keywords'.")
> +
> +(defcustom ruby-align-to-stmt-keywords '(def)
>    "Keywords after which we align the expression body to statement.
>
>  When nil, an expression that begins with one these keywords is
> @@ -250,17 +253,13 @@
>
>  Only has effect when `ruby-use-smie' is t.
>  "
> -  :type '(choice
> +  :type `(choice
>            (const :tag "None" nil)
>            (const :tag "All" t)
>            (repeat :tag "User defined"
> -                  (choice (const if)
> -                          (const while)
> -                          (const unless)
> -                          (const until)
> -                          (const begin)
> -                          (const case)
> -                          (const for))))
> +                  (choice ,@(mapcar
> +                             (lambda (kw) (list 'const kw))
> +                             ruby-alignable-keywords))))
>    :group 'ruby
>    :safe 'listp
>    :version "24.4")
> @@ -639,7 +638,7 @@
>            (smie-indent--hanging-p)
>            ruby-indent-level))
>      (`(:after . ,(or "?" ":")) ruby-indent-level)
> -    (`(:before . ,(or "if" "while" "unless" "until" "begin" "case" "for"))
> +    (`(:before . ,(guard (memq (intern token) ruby-alignable-keywords)))
>       (when (not (ruby--at-indentation-p))
>         (if (ruby-smie--indent-to-stmt-p token)
>             (ruby-smie--indent-to-stmt)
>
>

[-- Attachment #2: Type: text/html, Size: 4604 bytes --]

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Dmitry Gutov]

On 16.01.2014 12:15, Bozhidar Batsov wrote:
> I'm OK with the patch, but it makes configuration a bit more difficult
> since users should actually know all the alignable keywords.

Why? The Customize interface will still show all possible values, and if 
the user looks at the definition of the variable directly, they should 
notice the use of `ruby-alignable-keywords'.

> I guess we
> can mention `ruby-alignable-keywords' in the docstring
> of `ruby-align-to-stmt-keywords' and consider this a good enough hint
> for the users. One problem with the current implementation is that it
> won't play nice with assignments if you won't to treat them differently:

Do you suggest to both apply the patch I suggested, *and* add a new user 
option? Because without the patch, there won't be a way to treat them 
"the same".

> What I'm saying is that some people
> might prefer to align `def` with a statement beginning only with access
> modifier methods.

What I'd like to know if you personally plan to use such user option. 
Adapting to a hypothetical user can wait until the release of 24.4, I'd 
say. And it'd be better to wait until a specific feature request anyway.

 > Of course, it seems unlikely that someone will assign
> the value of a method def to a variable, but in the future a method
> definition in MRI,

The above sentence seems to be missing something.

 > but it makes sense in Rubinius for instance.

Could you explain why?

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]

On 16 January 2014 15:37, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 16.01.2014 12:15, Bozhidar Batsov wrote:
>
>> I'm OK with the patch, but it makes configuration a bit more difficult
>> since users should actually know all the alignable keywords.
>>
>
> Why? The Customize interface will still show all possible values, and if
> the user looks at the definition of the variable directly, they should
> notice the use of `ruby-alignable-keywords'.


Fair enough, although in practice relatively few people use it. Anyways,
this is a minor detail.

>
>
>  I guess we
>> can mention `ruby-alignable-keywords' in the docstring
>> of `ruby-align-to-stmt-keywords' and consider this a good enough hint
>> for the users. One problem with the current implementation is that it
>> won't play nice with assignments if you won't to treat them differently:
>>
>
> Do you suggest to both apply the patch I suggested, *and* add a new user
> option? Because without the patch, there won't be a way to treat them "the
> same".


By "current implementation" I meant the current patch. Yeah, I think that
some way to have defs align to the start of an expression only for modifier
expressions might be a good idea in the interest of maximum flexibility.


>
>
>  What I'm saying is that some people
>> might prefer to align `def` with a statement beginning only with access
>> modifier methods.
>>
>
> What I'd like to know if you personally plan to use such user option.
> Adapting to a hypothetical user can wait until the release of 24.4, I'd
> say. And it'd be better to wait until a specific feature request anyway.


It depends on whether I'd have to use the result of an method definition in
an assignment. :-) Seems unlikely at this point, since I don't have strong
feelings about this.


>
>
> > Of course, it seems unlikely that someone will assign
>
>> the value of a method def to a variable, but in the future a method
>> definition in MRI,
>>
>
> The above sentence seems to be missing something.


Can't believe I wrote something as crazy as this. I meant to say that in
the current implementation of the feature in MRI assigning the result to a
variable doesn't make much sense, but in the future that might change (if
the return value becomes a method object instead of a symbol).

>
>
> > but it makes sense in Rubinius for instance.
>
> Could you explain why?
>

In Rubinius defs have always returned an object:

def foo; end
=> #<Rubinius::CompiledMethod foo file=(irb)>

I can assume that some people capture this object and manipulate it.

Anyways, I think that the current patch will be good enough for most users.
I was just pointing out some potential problems in case they were
overlooked by you.

[-- Attachment #2: Type: text/html, Size: 4976 bytes --]

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Dmitry Gutov]

On 16.01.2014 16:26, Bozhidar Batsov wrote:
> By "current implementation" I meant the current patch. Yeah, I think
> that some way to have defs align to the start of an expression only for
> modifier expressions might be a good idea in the interest of maximum
> flexibility.

They are plain method calls anyway. Like mentioned, `private', etc, are 
not keywords.

> I meant to say that in
> the current implementation of the feature in MRI assigning the result to
> a variable doesn't make much sense, but in the future that might change
> (if the return value becomes a method object instead of a symbol).

I don't think it's likely to change. Previously, the return value was 
undefined. Now that it's specified to be method name symbol, it probably 
won't ever change, in the interests of backward compatibility.

> In Rubinius defs have always returned an object:
>
> def foo; end
> => #<Rubinius::CompiledMethod foo file=(irb)>

Ah, I see. It's probably considered an implementation detail, though.

> Anyways, I think that the current patch will be good enough for most
> users. I was just pointing out some potential problems in case they were
> overlooked by you.

Thanks, but the occasional need for more flexibility is often apparent. 
The question I'm usually trying to answer is whether we can get away 
with fewer options, and still keep users happy.

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

One thing I've learned from maintaining open-source projects is that
someone will always be unhappy no matter what you do. :-) The patch you've
suggested
improves upon the current state of the code and add extra flexibility, so
we probably shouldn't dwell too much over the details in it and simply
install it.


On 16 January 2014 20:40, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 16.01.2014 16:26, Bozhidar Batsov wrote:
>
>> By "current implementation" I meant the current patch. Yeah, I think
>> that some way to have defs align to the start of an expression only for
>> modifier expressions might be a good idea in the interest of maximum
>> flexibility.
>>
>
> They are plain method calls anyway. Like mentioned, `private', etc, are
> not keywords.
>
>
>  I meant to say that in
>> the current implementation of the feature in MRI assigning the result to
>> a variable doesn't make much sense, but in the future that might change
>> (if the return value becomes a method object instead of a symbol).
>>
>
> I don't think it's likely to change. Previously, the return value was
> undefined. Now that it's specified to be method name symbol, it probably
> won't ever change, in the interests of backward compatibility.
>
>
>  In Rubinius defs have always returned an object:
>>
>> def foo; end
>> => #<Rubinius::CompiledMethod foo file=(irb)>
>>
>
> Ah, I see. It's probably considered an implementation detail, though.
>
>
>  Anyways, I think that the current patch will be good enough for most
>> users. I was just pointing out some potential problems in case they were
>> overlooked by you.
>>
>
> Thanks, but the occasional need for more flexibility is often apparent.
> The question I'm usually trying to answer is whether we can get away with
> fewer options, and still keep users happy.
>

[-- Attachment #2: Type: text/html, Size: 2783 bytes --]

Re: [ruby-mode] Private/protected method definition layout in Ruby 2.1

[Dmitry Gutov]

On 16.01.2014 21:38, Bozhidar Batsov wrote:
> One thing I've learned from maintaining open-source projects is that
> someone will always be unhappy no matter what you do. :-) The patch
> you've suggested
> improves upon the current state of the code and add extra flexibility,
> so we probably shouldn't dwell too much over the details in it and
> simply install it.

Ok, done. :)