Add basic line length based formatting support #66

gingermusketeer · 2017-11-18T19:17:30Z

First step as part of #54.

Adds:

doc_printer: This is used to print the assembled document.
doc_builder: This is used to assemble a document for printing.
Support for multiple config options per test.
Ability to configure how long a line should be via print_width
Line length based formatting for arrays.

Remaining:

Heredoc handling (this is what is causing the last 5 tests to fail).

Why:

Why: So that other usages don't need to be fixed yet.

Why: So that I can workout what the formatter needs to construct.

mjago · 2017-11-20T09:56:33Z

@gingermusketeer This is looking great! 👍

danielma

This is looking great! I have a few small things I'd tweak, but no major hangups. This is a great step forward!

danielma · 2017-11-20T17:34:13Z

lib/rufo/formatter.rb

+      # arg1, ..., *star
+      doc = visit args
+    else
+      pre_doc, *_ = with_doc_mode {


For a multiline block, can we use do...end?

danielma · 2017-11-20T17:36:48Z

lib/rufo/formatter.rb

+      doc << B.concat([last, B.if_break(",", "")])
+    end
+
+    B.group(


This expression is really nice to read 👏.

In the end, I think this points to a more readable future for rufo. Instead of needing to keep the entire stack in your head to understand a point in code, we can capture output and express the ultimate shape of an expression at the end

danielma · 2017-11-20T17:38:46Z

lib/rufo/formatter.rb

+
+    elements.each_with_index do |elem, i|
+      doc_el = visit(elem)
+      if doc_el.is_a?(Array)


Nitpick: what do you think about coalescing every doc_el into an array? This can replace this entire if expression

doc.concat Array(doc_el)

I have tried this and it causes the tests to break. Perhaps we can revisit this once we are further down the implementation of prettier printer.

danielma · 2017-11-20T17:41:16Z

lib/rufo/settings.rb

      value = options.fetch(name, default)
      unless valid_options.include?(value)
        $stderr.puts "Invalid value for #{name}: #{value.inspect}. Valid " \
-                     "values are: #{valid_options.map(&:inspect).join(', ')}"
+                      "values are: #{valid_options.map(&:inspect).join(', ')}"


Why did a space get added here? Was it added manually, or did rufo do that in the formatting pass?

Um not sure to be honest. I will run the formatter over it once the tests are passing. Hopefully that will remove it.

gingermusketeer · 2017-11-22T09:25:27Z

@danielma Thanks for the feedback :)

mjago · 2017-11-22T09:31:47Z

@gingermusketeer just a heads-up whilst checking out your printer:
I noticed the following (print_width: 80):

[([1,2]), ([3,4]), ([5,6])]

formats to:

[), ), )]

Why: So that syntax highlighting will work in some editors.

bessey

Changes look good to me.

Why: content was being removed due to the regex being used.

Why: So that each type does not need to be wrapped in a capture_output block.

Why: Until this is used by other parts of the formatter we cannot handle anything before the array other than indentation.

Why: So that tests work on ruby 2.3.

…port

Why: to reduce code duplication.

gingermusketeer · 2017-12-03T11:42:20Z

@bessey Apologies for the slow response. Yes your understanding is correct :)

I have all the tests passing now that heredocs are finally working. Only concern is this pending test: https://github.com/ruby-formatter/rufo/pull/66/files#diff-1be4af1647655801c693a52968d5d64dR754

The changes causes any multiline method calls inside an array to have incorrect indentation. I am hesitant to try and tackle that in this PR due to how big this getting. I can see a couple options for how we proceed:

Merge into master and then work on fixing the problem and divide and conquer.
- Won't be able to release a new version of the gem until this happens (perhaps we should do a release prior?)
Merge into another branch and then divide and conquer.
Fix on this branch.

Thoughts?

danielma · 2017-12-11T18:33:09Z

✋ vote for

Merge into master and then work on fixing the problem and divide and conquer.

danielma · 2017-12-11T19:22:26Z

FYI, I'm working on a branch right now that would fix that indent error. Trying to get familiar with using this doc building code

mjago · 2017-12-11T21:08:14Z

I've been talking to @gingermusketeer on Gitter this afternoon and we feel it would probably be better in the long run to develop a new formatter along side the old one, in a branch called new-formatter - complete with its own specs. Once complete the new formatter can be switched into prime time. This allows the current formatter to be maintained and receive bug fixes as reported in isolation to the new formatter work. The idea is to strip the new formatter of much of the old `wiring mechanism' and build up the doc-based formatter piece by piece, working back up from the simpler sexp structures to the more complex structures containing the simpler structures.

bessey · 2017-12-12T09:22:33Z

Works for me! Ps I'm pretty busy over the Xmas period so haven't been very active here, but I'll be back in January.

…

On Mon, 11 Dec 2017, 21:09 Martyn Jago, ***@***.***> wrote: I've been talking to @gingermusketeer <https://github.com/gingermusketeer> on Gitter this afternoon and we feel it would probably be better in the long run to develop a new formatter along side the old one, in a branch called new-formatter - complete with its own specs. Once complete the new formatter can be switched into prime time. This allows the current formatter to be maintained and receive bug fixes as reported in isolation to the new formatter work. The idea is to strip the new formatter of much of the old `wiring mechanism' and build up the doc-based formatter piece by piece, working back up from the simpler sexp structures to the more complex structures containing the simpler structures. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArOaJjAC4xTx5DmY4aN2J3ltyoBcfoOks5s_ZoBgaJpZM4QjHW4> .

bessey · 2018-01-25T03:42:33Z

Any news here? I notice the new-formatter branch is missing this work, perhaps someone hasn't pushed in a while?

gingermusketeer · 2018-01-26T11:37:28Z

@bessey I will merge this branch into there now :)

gingermusketeer added 24 commits November 18, 2017 19:35

Add line length restrained printing utilities

6470ebf

Fix tests for older versions of ruby

a794fbb

Add more complex example and fix indent printing

d4558e1

Add initial prettier printing integration

ca3a59a

Add doc mode helpers

fbe2c42

Return token from consume_token

94ddb11

Allow print_width to be configured

6b2895b

Fix more tests

656636f

Why:

Fix test for comment on first element

85e6165

Why:

Fix remaining trailing comma tests

8867e8b

Why:

Fix simple method call spec

fdd6242

Fix percent array literal test

a93cd49

Fix simple array specs

903e72a

Add support for star args

df24550

Fix more complex tests

6c90829

Fix hash test

46cc5fd

Only use new args_add_star logic in doc mode

8b015d6

Why: So that other usages don't need to be fixed yet.

Fix comment on a newline after an array item

130bc96

Handle strings in doc mode

4e52126

Change specs to match new formatting

befd18d

Fix nested array issue

5d50eb6

Add printer example for heredoc

2d7b873

Why: So that I can workout what the formatter needs to construct.

Remove unneeded code

68113ea

Cleanup

de073df

danielma approved these changes Nov 20, 2017

View reviewed changes

gingermusketeer added 2 commits November 22, 2017 22:11

Address PR feedback

53e5eee

Add example of nested array with brackets

d5bed76

gingermusketeer added 2 commits November 27, 2017 20:53

Switch from CODE to RUBY for heredocs

c607c78

Why: So that syntax highlighting will work in some editors.

Cleanup unused code

7944964

bessey approved these changes Nov 28, 2017

View reviewed changes

gingermusketeer added 8 commits December 2, 2017 04:55

Fix printing heredocs in printer

84f5f0a

Why: content was being removed due to the regex being used.

Fix heredoc handling for doc printer

5f97fdd

Add new test cases and fix them

e71def6

Refactor to handle non doc handled nodes generically

711bfd5

Why: So that each type does not need to be wrapped in a capture_output block.

Fix print_width config calculation

7fed281

Why: Until this is used by other parts of the formatter we cannot handle anything before the array other than indentation.

Format codebase

e25138d

Remove usage of squiggly heredocs

8cc334d

Why: So that tests work on ruby 2.3.

Add test for indentation issue

ba42b5c

gingermusketeer force-pushed the add-basic-line-length-based-formatting-support branch from f6ab762 to ba42b5c Compare December 2, 2017 14:06

gingermusketeer added 7 commits December 2, 2017 10:27

Merge branch 'master' into add-basic-line-length-based-formatting-sup…

c8ce87f

…port

Refactor literal elements method

46ea2f4

Why: to reduce code duplication.

Remove unneeded code

99901fd

Refactor comment handling

660e797

Code style tweaks

7ff6caf

Fix array literal bug

c7028c2

Handle percent array nested inside normal array

4835c73

gingermusketeer changed the base branch from master to new-formatter December 11, 2017 15:28

gingermusketeer merged commit 7bb9e85 into new-formatter Jan 26, 2018

gingermusketeer deleted the add-basic-line-length-based-formatting-support branch January 26, 2018 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic line length based formatting support #66

Add basic line length based formatting support #66

gingermusketeer commented Nov 18, 2017 •

edited

Loading

mjago commented Nov 20, 2017

danielma left a comment

danielma Nov 20, 2017

danielma Nov 20, 2017

danielma Nov 20, 2017

gingermusketeer Dec 2, 2017

danielma Nov 20, 2017

gingermusketeer Nov 22, 2017

gingermusketeer commented Nov 22, 2017

mjago commented Nov 22, 2017

bessey left a comment

gingermusketeer commented Dec 3, 2017

danielma commented Dec 11, 2017

danielma commented Dec 11, 2017 •

edited

Loading

mjago commented Dec 11, 2017

bessey commented Dec 12, 2017 via email

bessey commented Jan 25, 2018

gingermusketeer commented Jan 26, 2018

Add basic line length based formatting support #66

Add basic line length based formatting support #66

Conversation

gingermusketeer commented Nov 18, 2017 • edited Loading

mjago commented Nov 20, 2017

danielma left a comment

Choose a reason for hiding this comment

danielma Nov 20, 2017

Choose a reason for hiding this comment

danielma Nov 20, 2017

Choose a reason for hiding this comment

danielma Nov 20, 2017

Choose a reason for hiding this comment

gingermusketeer Dec 2, 2017

Choose a reason for hiding this comment

danielma Nov 20, 2017

Choose a reason for hiding this comment

gingermusketeer Nov 22, 2017

Choose a reason for hiding this comment

gingermusketeer commented Nov 22, 2017

mjago commented Nov 22, 2017

bessey left a comment

Choose a reason for hiding this comment

gingermusketeer commented Dec 3, 2017

danielma commented Dec 11, 2017

danielma commented Dec 11, 2017 • edited Loading

mjago commented Dec 11, 2017

bessey commented Dec 12, 2017 via email

bessey commented Jan 25, 2018

gingermusketeer commented Jan 26, 2018

gingermusketeer commented Nov 18, 2017 •

edited

Loading

danielma commented Dec 11, 2017 •

edited

Loading