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

discussion: "aligning of arrays" (or other things) #119

Closed
jorroll opened this issue Aug 31, 2018 · 2 comments
Closed

discussion: "aligning of arrays" (or other things) #119

jorroll opened this issue Aug 31, 2018 · 2 comments

Comments

@jorroll
Copy link

jorroll commented Aug 31, 2018

Trying out rufo in my project, I like it with one dealbreaking exception: the apparent forced "aligning of arrays" (or other things). I imagine this is something that has been discussed before, but I couldn't find a specific issue.

Rufo changes this code:

class Email
  class Schema
    class RemoveRecipient < ApplicationSchema

      define do |schema|
        schema.required(:property).schema do
          required(:recipient_type, Types::Coerce::String).filled(included_in?: [
            "THIS IS A LONG STRING OF TEXT THAT REALLY SHOULDN'T BE PUSHED TO THE RIGHT",
            "CC",
            "BCC",
          ])
          required(:email_address, Types::Email).filled
        end

      end
    end
  end
end

To this, which contains an unacceptable line length for me (it actually also removes the extra line breaks, but I've added them back in for readability):

class Email
  class Schema
    class RemoveRecipient < ApplicationSchema

      define do |schema|
        schema.required(:property).schema do
          required(:recipient_type, Types::Coerce::String).filled(included_in?: [
                                                                    "THIS IS A LONG STRING OF TEXT THAT REALLY SHOULDN'T BE PUSHED TO THE RIGHT",
                                                                    "CC",
                                                                    "BCC",
                                                                  ])
          required(:email_address, Types::Email).filled
        end

      end
    end
  end
end

I see issue #54 is already dealing with line length based formatting, which maybe this would fall under, but I wanted to call out this case specifically because, so far as I can tell, there's currently no way to stop the above behavior.

Is the above considered a bug? Desired behavior?

I would argue that consistency in formatting is important, and because the above is unacceptable, the only solution that will work consistently is to never "align arrays". The same logic would apply to "aligning" other items like case statements:

I.e. this:

this_is_a_long_veriable_name = case some_variable
when :one
  "THIS IS A REALLY LONG STRING THAT REALLY SHOULDN'T BE PUSHED TO THE RIGHT"
when :two
  "one"
end

Is transformed to this:

this_is_a_long_veriable_name = case some_variable
                               when :one
                                 "THIS IS A REALLY LONG STRING THAT REALLY SHOULDN'T BE PUSHED TO THE RIGHT"
                               when :two
                                 "one"
                               end

Personally, in all these cases I could accept a minor level of consistent indentation, the exact amount of which could be decided in a separate issue:
I.e. something like

this_is_a_long_veriable_name = case some_variable
    when :one
      "THIS IS A REALLY LONG STRING THAT REALLY SHOULDN'T BE PUSHED TO THE RIGHT"
    when :two
      "one"
    end

Anyway, obviously this is your project, and I definitely won't press the issue if you decide to keep things as is.

It's also possible I could accept inconsistant formatting, where rufo tried to "align things", but would fall back on other behavior if that produced a line length that is too long. I'd really question this, specific, choice though (i.e. why not just choose the consistent option?).

@jorroll
Copy link
Author

jorroll commented Aug 31, 2018

Somewhat related: I heard about rufo from this PR in graphql-ruby, which also mentions the issue described here as a possible dealbreaker.

@jorroll
Copy link
Author

jorroll commented Aug 31, 2018

Oops! I found an existing issue for this #90

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

No branches or pull requests

1 participant