Private/protected indenting #179

Merged
merged 10 commits into from Nov 3, 2013

Conversation

Projects
None yet
2 participants
@AndrewRadev
Member

AndrewRadev commented Oct 19, 2013

This implements different indenting for private/protected scopes, depending on an option (g:ruby_indent_private_protected_style). The original issue is #177.

This has yet to be documented and should be tested thoroughly before merging.

AndrewRadev added some commits Oct 19, 2013

Also add an "outdent" option
Not requested at this time, but easy enough to implement.
@drn

This comment has been minimized.

Show comment
Hide comment
@drn

drn Oct 22, 2013

Contributor

Thanks for implementing this @AndrewRadev !

Here's a quick screencast of some testing I did with all of my plugins except vim-ruby disabled -
http://screencast.com/t/QwykEEqZb

You'll notice that for the 'indent' and 'outdent' settings, the same thing is happening. I'll do some more testing tomorrow and get back to you with more info.

Contributor

drn commented Oct 22, 2013

Thanks for implementing this @AndrewRadev !

Here's a quick screencast of some testing I did with all of my plugins except vim-ruby disabled -
http://screencast.com/t/QwykEEqZb

You'll notice that for the 'indent' and 'outdent' settings, the same thing is happening. I'll do some more testing tomorrow and get back to you with more info.

@AndrewRadev

This comment has been minimized.

Show comment
Hide comment
@AndrewRadev

AndrewRadev Oct 22, 2013

Member

I've pushed a fix for the issue, I had forgotten to readjust indent while typing. One other thing I have yet to implement is how "public" should behave. For the "outdent" style, it should probably also outdent that one, but not for the "indent" one.

Member

AndrewRadev commented Oct 22, 2013

I've pushed a fix for the issue, I had forgotten to readjust indent while typing. One other thing I have yet to implement is how "public" should behave. For the "outdent" style, it should probably also outdent that one, but not for the "indent" one.

@drn

This comment has been minimized.

Show comment
Hide comment
@drn

drn Oct 23, 2013

Contributor

Cool - working a lot better now. I've found a few other issues:

Indentation works well for non-nested classes, however trying to indent after a nested class results in the indentation of the nearest class definition, instead of the parent class definition. eg:

class Outer
  class Inner
    private
      def method; end
    protected
      def method; end
    private :method
    protected :method
  end

    private
    protected
    private :method
    protected :method
end

Is it feasible to instead of using the nearest class definition's indentation, identify and use the parent class definition? Ideally, the class indentation search would first filter out all the indentation groups (class..end, def..end, etc) and then return the line number of the parent class definition.

The outdent setting auto-outdents private :method as soon as the last letter of private is typed. I tried looking into the indentkeys configuration and couldn't figure out a way to do this - is there any way to have the indentkeys indent only on the after private.*?

Contributor

drn commented Oct 23, 2013

Cool - working a lot better now. I've found a few other issues:

Indentation works well for non-nested classes, however trying to indent after a nested class results in the indentation of the nearest class definition, instead of the parent class definition. eg:

class Outer
  class Inner
    private
      def method; end
    protected
      def method; end
    private :method
    protected :method
  end

    private
    protected
    private :method
    protected :method
end

Is it feasible to instead of using the nearest class definition's indentation, identify and use the parent class definition? Ideally, the class indentation search would first filter out all the indentation groups (class..end, def..end, etc) and then return the line number of the parent class definition.

The outdent setting auto-outdents private :method as soon as the last letter of private is typed. I tried looking into the indentkeys configuration and couldn't figure out a way to do this - is there any way to have the indentkeys indent only on the after private.*?

@AndrewRadev

This comment has been minimized.

Show comment
Hide comment
@AndrewRadev

AndrewRadev Oct 23, 2013

Member

Is it feasible to instead of using the nearest class definition's indentation, identify and use the parent class definition? Ideally, the class indentation search would first filter out all the indentation groups (class..end, def..end, etc) and then return the line number of the parent class definition.

Yeah, I guess I'll have to pull some searchpair trickery. I'll see what I can do about it.

The outdent setting auto-outdents private :method as soon as the last letter of private is typed. I tried looking into the indentkeys configuration and couldn't figure out a way to do this - is there any way to have the indentkeys indent only on the after private.*

I don't know of any, either, I'm not very familiar with indentkeys, to be honest. I'll investigate as well, although if it turns out to be difficult, I may drop the "outdent" functionality altogether for now.

I'll try to continue working on this the upcoming weekend. If you notice other issues or have ideas for fixes, do let me know.

Member

AndrewRadev commented Oct 23, 2013

Is it feasible to instead of using the nearest class definition's indentation, identify and use the parent class definition? Ideally, the class indentation search would first filter out all the indentation groups (class..end, def..end, etc) and then return the line number of the parent class definition.

Yeah, I guess I'll have to pull some searchpair trickery. I'll see what I can do about it.

The outdent setting auto-outdents private :method as soon as the last letter of private is typed. I tried looking into the indentkeys configuration and couldn't figure out a way to do this - is there any way to have the indentkeys indent only on the after private.*

I don't know of any, either, I'm not very familiar with indentkeys, to be honest. I'll investigate as well, although if it turns out to be difficult, I may drop the "outdent" functionality altogether for now.

I'll try to continue working on this the upcoming weekend. If you notice other issues or have ideas for fixes, do let me know.

@drn

This comment has been minimized.

Show comment
Hide comment
@drn

drn Oct 24, 2013

Contributor

Great, hope the searchpair work will do the trick.

After browsing the indentkeys docs, I found that this fixes our outdent issue ( #182 ). This updates indentation as soon as a : is typed after the label private or protected. What do you think?

I'll keep you updated if I find any other issues.

Contributor

drn commented Oct 24, 2013

Great, hope the searchpair work will do the trick.

After browsing the indentkeys docs, I found that this fixes our outdent issue ( #182 ). This updates indentation as soon as a : is typed after the label private or protected. What do you think?

I'll keep you updated if I find any other issues.

@AndrewRadev

This comment has been minimized.

Show comment
Hide comment
@AndrewRadev

AndrewRadev Oct 27, 2013

Member

I've pushed a commit that should make the indent script find the "correct" class definition. Turned out to be easier than I thought with the existing infrastructure, though it still needs to be thoroughly tested, of course.

Regarding the : indenting, I've merged your commit, although I've also commented on #182, let me know about that.

The one thing that I'm still not sure of is indenting of public. I would say that this would be the "correct" approach:

# style = "indent"
#
class Example
  def one
  end

  public

  def two
  end

  protected

    def three
    end

  private

    def four
    end
end

# style = "outdent"
#
class Example
  def one
  end

public

  def two
  end

protected

  def three
  end

private

  def four
  end
end

This would mean that outdent affects it, while indent doesn't. I guess the rails convention doesn't concern itself with the public case. In any case, let me know about your thoughts on this.

Regardless of whether public is touched or not, it seems like it could be a topic of change in the future, so I think it would be more appropriate to call the option ruby_indent_access_modifier_style. As in,

let g:ruby_indent_access_modifier_style = 'normal'
let g:ruby_indent_access_modifier_style = 'indent'
let g:ruby_indent_access_modifier_style = 'outdent'

Any objections by anyone on the naming are welcome, preferably as early as possible :).

Member

AndrewRadev commented Oct 27, 2013

I've pushed a commit that should make the indent script find the "correct" class definition. Turned out to be easier than I thought with the existing infrastructure, though it still needs to be thoroughly tested, of course.

Regarding the : indenting, I've merged your commit, although I've also commented on #182, let me know about that.

The one thing that I'm still not sure of is indenting of public. I would say that this would be the "correct" approach:

# style = "indent"
#
class Example
  def one
  end

  public

  def two
  end

  protected

    def three
    end

  private

    def four
    end
end

# style = "outdent"
#
class Example
  def one
  end

public

  def two
  end

protected

  def three
  end

private

  def four
  end
end

This would mean that outdent affects it, while indent doesn't. I guess the rails convention doesn't concern itself with the public case. In any case, let me know about your thoughts on this.

Regardless of whether public is touched or not, it seems like it could be a topic of change in the future, so I think it would be more appropriate to call the option ruby_indent_access_modifier_style. As in,

let g:ruby_indent_access_modifier_style = 'normal'
let g:ruby_indent_access_modifier_style = 'indent'
let g:ruby_indent_access_modifier_style = 'outdent'

Any objections by anyone on the naming are welcome, preferably as early as possible :).

@drn

This comment has been minimized.

Show comment
Hide comment
@drn

drn Nov 1, 2013

Contributor

@AndrewRadev - Sorry for the late reply! I've been busy at work this past week

As for the : indentkey, you are indeed correct. I thought it was reindenting after a : only if a private and protected label was present in the same line, but that doesn't seem to be the case.

Anyway, I decided to try my hand at some vimscript - #184
I implemented your suggested change to the ruby_indent_access_modifier_style option, implemented support for public, iterated on the tests, and updated the README.

What do you think?

Contributor

drn commented Nov 1, 2013

@AndrewRadev - Sorry for the late reply! I've been busy at work this past week

As for the : indentkey, you are indeed correct. I thought it was reindenting after a : only if a private and protected label was present in the same line, but that doesn't seem to be the case.

Anyway, I decided to try my hand at some vimscript - #184
I implemented your suggested change to the ruby_indent_access_modifier_style option, implemented support for public, iterated on the tests, and updated the README.

What do you think?

drn and others added some commits Nov 1, 2013

Remove redundant s:GetMSL()
In the case of an access modifier, there shouldn't be the need to find
an MSL, since the line itself couldn't be a part of a continuation.
@AndrewRadev

This comment has been minimized.

Show comment
Hide comment
@AndrewRadev

AndrewRadev Nov 3, 2013

Member

Well, this looks good to me. I've made some other minor adjustments, the biggest of which is the removal of a s:GetMSL call, which (I believe) shouldn't be necessary.

With this, I think the feature is ready to merge. The changes are mostly optional, the only global difference is the indentkeys. Hopefully this won't have any surprising effects, but I guess if it does, we'll get issue reports.

@DarrenLi, if you notice some problem with my recent changes (or any regression that looks related), feel free to reopen the issue. And thanks again for your help :).

Member

AndrewRadev commented Nov 3, 2013

Well, this looks good to me. I've made some other minor adjustments, the biggest of which is the removal of a s:GetMSL call, which (I believe) shouldn't be necessary.

With this, I think the feature is ready to merge. The changes are mostly optional, the only global difference is the indentkeys. Hopefully this won't have any surprising effects, but I guess if it does, we'll get issue reports.

@DarrenLi, if you notice some problem with my recent changes (or any regression that looks related), feel free to reopen the issue. And thanks again for your help :).

AndrewRadev added a commit that referenced this pull request Nov 3, 2013

@AndrewRadev AndrewRadev merged commit 2597860 into master Nov 3, 2013

@AndrewRadev AndrewRadev deleted the private-protected-indenting branch Nov 3, 2013

@drn

This comment has been minimized.

Show comment
Hide comment
@drn

drn Nov 3, 2013

Contributor

@AndrewRadev - Great! Seems to work well. Thanks for the help. Glad we got this merged 👍

Contributor

drn commented Nov 3, 2013

@AndrewRadev - Great! Seems to work well. Thanks for the help. Glad we got this merged 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment