Comments

[kddnewton]

Currently, comment handling is done through the wrong abstraction and needs to be refactored. We currently use the ripper lexer state to determine which comment node to attach to, and then have comments printed out generically in the main printer. This results in massive amounts of weird errors.

For examples, see #100, #128, #149, #150, #175, #176, #177, #238, #274, #294, #332, #341, #343, #353, #391, #405, #408, #558, #564, #565, #571, #572, #576.

All of these can be fixed by attaching comments after the nodes have been built. The basic flow is going to be to parse out the comments but not attach them directly to the nodes. Instead to make a big list of them and attach them appropriately afterward. This is going to involve going through a ton of different examples and checking them by hand. For now, I'm going to close all of those issues in favor of tracking everything here.

[kddnewton]

Below here are all of the failing examples I could find. I pulled them out of the existing issues.

Click to expand

TEST = [
  "1", # 1
  "2"  # 2
]

def my_method!(some_arg)
  some_thing[:some_key] = {
    my_val: some_arg.my_val || 'a', # TODO: Remove this hard coding
    other_val: my_method(some_arg.other_val),
    other_stuff: {
      banana: some_arg.banana
    }
  }
end

x = [
  1,
  2,
  3  # hello world
]

x = 
  1, # first
  2, # second
  3  # third

list.each do |item|
  # Some comment
  puts item[0]
end

class DeviseCreateUsers < ActiveRecord::Migration[5.2]
  def change
    create_table :users do |t|
      # Database Authenticatable
      t.string :email, null: false, default: ''
      t.string :encrypted_password, null: false, default: ''

      # ...
    end
  end
end

x = {
  my_val: 1, # TODO: Remove this hard coding
  other_val: my_method(2)
}

expect(person).to have_attributes(
  :name => 'Alice',
  :email => 'alice.white@example.com', # comment
)

# action_methods are cached and there is sometimes need to refresh
# them. ::clear_action_methods! allows you to do that, so next time
# you run action_methods, they will be recalculated.
def clear_action_methods! # :doc:
  @action_methods = nil
end

def foo!() # :doc:
  # hello world
  1
end

{
  some_key: :some_value,
  another_key: :another_value # long comment with lots of words, like yes, a lot of words, I mean a lot
}

array = [ # comment
  0 # comment
]

class RackAttack
  Rack::Attack.throttled_response = lambda do |env|
    # NB: you have access to the name and other data about the matched throttle
    #  env['rack.attack.matched'],
    #  env['rack.attack.match_type'],
    #  env['rack.attack.match_data'],
    #  env['rack.attack.match_discriminator']

    # Using 503 because it may make attacker think that they have successfully
    # DOSed the site. Rack::Attack returns 429 for throttling by default
    [ 429, {}, ["Retry again in 2 minutes or contact support for further assistance"] ]
  end
end

def index
  if true
    do_stuff
  else
    # comment
    do_stuff

    # comment
    do_stuff

    # comment
    do_stuff

    # comment
    do_stuff

    if true
      do_stuff
    else
      do_stuff
    end
  end
end

{
  x: foo(
    y: 1 # comment
  )
}

DeviseTokenAuth.setup do |config|
  # config.enable_standard_devise_support = false
end

{
  **foo,
  # some comment
  **bar,
}

# this is from graphql-ruby
context = {
  # Query context goes here, for example:
  # current_user: current_user,
}

# however, in this case the comment is not lost:
context = {
  # first line of comment
  # second line of comment
}

def x
  a do |b|
    # CSV Format
    user full_name: 'Member'
    device "Type" do |device| device.member_type end
  end
end

class Integration < ActiveRecord::Base
  def week(now = Time.current)
    case created_at.wday
    when 0 #sund
      created_at.next_week.beginning_of_week
    when 1 #monday
      created_at.beginning_of_week
    when 2 #tues
      created_at.beginning_of_week
    when 3 #wed
      created_at.beginning_of_week
    when 4 #thur
      created_at.beginning_of_week
    when 5 #fri
      created_at.next_week.beginning_of_week
    when 6 #sat
      created_at.next_week.beginning_of_week
    end
  end
end

def testfunc(path)
  url = ''
  arrpath = path.split('/')
  arrpath.shift #comment1
  return url.chop #comment2
end

add_callback :start_binary do |node|
  # Consider anything used in an expression like "A or B" as used
  if %w[&& || and or].include?(node[2].to_s)
     test
   end
end

    locals(
      booking.update(event_registration_params),
      'User registration successfully updated!',
      booking,
      # This is a comment
      administration: assigned_to_self? && first_incomplete_assessment(booking),
    )

def myfunc(**kwargs)
  pp kwargs
end

myfunc(
  foo: 1,
  bar: 2 # a comment
)

foo.bar(
  bar: baz,
  # this is a comment at the end of the method call
)

SimpleForm.setup do |config|
  # Wrappers are used by the form builder to generate a
  # complete input. You can remove any component from the
  # wrapper, change the order or even add your own to the
  # stack. The options given below are used to wrap the
  # whole input.
  config.wrappers :default,
                  class: :input,
                  hint_class: :field_with_hint,
                  error_class: :field_with_errors,
                  valid_class: :field_without_errors do |b|
  end
end

xs.map do |x|
  # foo
  bar.baz :gorp, abc: 'def'
end

foo.bar(
  bar: baz,
  # this is a comment at the end of the method call
)

case foo
when 1
  true # bar
end

[AlanFoster]

@kddeisz Let me know if there's any work to help with here 👍

[johannesluedke]

@kddeisz It would be really nice to have a solution for this soon - the current handling of comments is quite annoying. Maybe one could add an option to disable messing with comments for now?

[kddnewton]

@johannesluedke sorry it's annoying. I'm working on this on the weekends whenever I get the chance. I'd be happy to look at a PR with that option - that'd be a great addition!

[johannesluedke]

@kddeisz Hi -- Sorry about the tone from the last comment.

Thank you a lot for contributing for working on rbprettier plugin -- apart from that issue it's really nice to work with it!

As for a PR: I am not sure if I can do that - can you point me to documentation of how to add an option? Is there maybe an older PR that added an option?

[TomerAberbach]

I'm not sure if this issue addresses this so I thought I'd bring it up. The plugin is also not idempotent.

For example, given the following input:

##
# An abstract class representing a controller which handles HTTP requests.
class ApplicationController < ActionController::Base

  # Enables request forgery protection
  protect_from_forgery with: :null_session
end

We get the following output:

##
# An abstract class representing a controller which handles HTTP requests.
class ApplicationController < ActionController::Base
  # Enables request forgery protection
  protect_from_forgery with: :null_session
end

However, if we run the output through prettier again we get a different output:

##
# An abstract class representing a controller which handles HTTP requests.
class ApplicationController < ActionController::Base # Enables request forgery protection
  protect_from_forgery with: :null_session
end

I think invoking the plugin on its own output should yield its own output.

[buckmaxwell]

Just running into this. Any current status update or branch I could look at with a start of a fix?

[kddnewton]

@buckmaxwell you can check out the better-comments branch.

[gitjul]

is there a way to disable moving comments whatsoever? it's really irritating and e.g. prevents adding rubocop comments.
as a configuration setting or override, that is. I expected to find a rule I can turn off in .prettierrc but I failed to do so.

[buckmaxwell]

Honestly that would probably be the best solution. As it is now it's just downright broken.

…

On Fri, Nov 6, 2020, 1:33 PM Julia Jacobs ***@***.***> wrote: is there a way to disable moving comments whatsoever? it's really irritating and e.g. prevents adding rubocop comments — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/prettier/plugin-ruby/issues/370*issuecomment-723234760__;Iw!!KGKeukY!nZTVU_A0qYkDn9S0yKVneHxbW9vNtzuuw347Mn3GkKBUug-Itx_5sIqEEZ-RrNGSlQ$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABPMHFG3WL6MRPKOV2VUC2TSOQ6PZANCNFSM4IJXEHBA__;!!KGKeukY!nZTVU_A0qYkDn9S0yKVneHxbW9vNtzuuw347Mn3GkKBUug-Itx_5sIqEEZ_Au5MItQ$> .

[kddnewton]

If either of y’all have time to open a PR I would immediately approve that. If not, you’re gonna have to wait. Got a new kid at home and need to do that for a while before I come back to this.

On Fri, Nov 6, 2020 at 3:57 PM Max Buck ***@***.***> wrote: Honestly that would probably be the best solution. As it is now it's just downright broken. On Fri, Nov 6, 2020, 1:33 PM Julia Jacobs ***@***.***> wrote: > is there a way to disable moving comments whatsoever? it's really > irritating and e.g. prevents adding rubocop comments > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < https://urldefense.com/v3/__https://github.com/prettier/plugin-ruby/issues/370*issuecomment-723234760__;Iw!!KGKeukY!nZTVU_A0qYkDn9S0yKVneHxbW9vNtzuuw347Mn3GkKBUug-Itx_5sIqEEZ-RrNGSlQ$ >, > or unsubscribe > < https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABPMHFG3WL6MRPKOV2VUC2TSOQ6PZANCNFSM4IJXEHBA__;!!KGKeukY!nZTVU_A0qYkDn9S0yKVneHxbW9vNtzuuw347Mn3GkKBUug-Itx_5sIqEEZ_Au5MItQ$ > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#370 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABG3P3QDDMSJ4AV6OINXRHTSORPNNANCNFSM4IJXEHBA> .

-- *Kevin D. Deisz* Staff Production Engineer Shopify <https://www.shopify.com/>

[gitjul]

@kddeisz congrats & good luck! 😄 👶

(no time for a PR atm but nice to know you're open to that solution)

[liskdev]

@kddeisz Not sure if this may help. Jumping comments after prettier parsing appeared after system ruby upgraded to 2.7
When i downgraded it to 2.5 it leaved comments untouched.

[kddnewton]

Very happy to report that every example listed in this issue is now fixed. It'll go out in the next release.

[gitjul]

was a rule introduced to disable moving comments?

[kddnewton]

There isn't an option, but if you see comments flying about, definitely feel free to open an issue.

[gitjul]

ok, will do that. but could there be an option to simply disable moving comments, as I suggested a few months back?

[kddnewton]

Not really, that's not really how this plugin works. When the code is parsed it gets transformed into an AST, then a prettier intermediate representation, and then that resulting tree gets printed. There's not really a way to say "don't move comments".