Fix up the reflow behavior of MD lists, a bunch of comment styles, and ascii email inclusions #45

satchm0h · 2016-03-08T15:47:14Z

Changes to address issues #4, #36, #39, #43

Fixed reflow of lists with '-' line prefix
- Added test
Fixed reflow of '/*' block comments
- Added test
Added support for reflow of '%' single line comments
- Added test
Added support for reflow of ';;' single line comments
- Added test
Added support for reflow of '>' ascii email inclusions
- Added test
Added test for '#' single line comments
Added test for '//' single line comments

- Fixed reflow of lists with '-' line prefix - Added test - Fixed reflow of '/*' block comments - Added test - Added support for reflow of '%' single line comments - Added test - Added support for reflow of ';;' single line comments - Added test - Added support for reflow of '>' ascii email inclusions - Added test - Added test for '#' single line comments - Added test for '//' single line comments

satchm0h · 2016-03-18T18:21:13Z

Any thoughts on getting this reviewed?

50Wliu · 2016-03-28T14:00:57Z

Any thoughts on getting this reviewed?

Unfortunately there's more open PRs than the Atom team can review, so there's currently a long backup. It might take some time until someone can give this a thorough review.

50Wliu · 2016-03-28T14:01:46Z

lib/autoflow.coffee

@@ -68,11 +68,22 @@ module.exports =
      currentLine = []
      currentLineLength = linePrefixTabExpanded.length

+      notFirstLine = false


I think it would be easier to understand if this was switched: firstLine = true

…ents from Wliu

satchm0h · 2016-05-05T17:56:03Z

Ping?

mturilli · 2016-10-28T23:08:32Z

Thank you satchm0h. This patch is relevant to my activity, I am happy to help however I can with the review process.

mdlinville · 2016-11-16T17:17:29Z

I would love to see this get in. I looked over the changes and they look great. If you can tell me how to test this locally, I'd love to do it.

50Wliu · 2016-11-16T21:18:31Z

@mstanleyjones apm dev autoflow [directory]. Then open Atom using atom --dev.

alflanagan · 2017-09-19T19:57:11Z

It's ... stunning ... that this simple fix has been under needs-review for a year and a half. How does one get approved to do reviews? Meanwhile I'm trying package magic-reflow.
UPDATE: apparently I can review, so I'm gearing up to do that. magic-reflow unfortunately didn't solve the use cases I have.

satchm0h · 2017-09-27T15:37:38Z

I've just given up on it being accepted at this point.

…

On Tue, Sep 19, 2017 at 3:57 PM, A. Lloyd Flanagan ***@***.*** > wrote: It's ... stunning ... that this simple fix has been under needs-review for a year and a half. How does one get approved to do reviews? Meanwhile I'm trying package magic-reflow <https://atom.io/packages/magic-reflow>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABt3t4UVHg_fdcYXGyAvjCk1XAF8HsE3ks5skByYgaJpZM4Hr3Jf> .

maxbrunsfeld

Sorry that it took forever for us to get to this. I left a couple of comments.

maxbrunsfeld · 2017-09-28T17:54:06Z

spec/autoflow-spec.coffee

@@ -311,3 +311,145 @@ describe "Autoflow package", ->
      '''

      expect(autoflow.reflow(text, wrapColumn: 2)).toEqual res
+
+    it 'properly reflows // comments ', ->
+      # Because there're known problems with this character in major regex engines


Does this comment apply to this example? It seems like it was copied over from a different test, but I don't think regex engines have problems with / characters.

maxbrunsfeld · 2017-09-28T17:57:43Z

lib/autoflow.coffee

+              linePrefix = linePrefix.replace(/^(\s*)\/\*/, '$1  ')
+            # Handle - list items
+            else if linePrefix.search(/^\s*-/) isnt -1
+              linePrefix = linePrefix.replace(/^(\s*)[\/#*-]/, '$1 ')


It looks like this processing of linePrefix could be moved outside of the loop; it doesn't depend on the loop variable segment. It looks like you've done it here because you need a different line prefix for the first line than for the remainder of the lines. Maybe before the loop you could define a second variable called wrappedLinePrefix.

I don't think it's necessary to first check that linePrefix matches a regex before calling replace on it with that regex. It's ok to call replace with a non-matching regex; it will just return the string unchanged.

Maybe something like this?

wrappedLinePrefix = linePrefix .replace(/^(\s*)\/\*/, '$1 ') .replace(/^(\s*)-/, '$1 ') firstLine = true for segment in @segmentText(blockLines.join(' ')) if @wrapSegment(segment, currentLineLength, wrapColumn) if firstLine # use `linePrefix` else # use `wrappedLinePrefix`

50Wliu · 2017-11-15T10:10:13Z

@satchm0h are you interested in addressing @maxbrunsfeld's comments?

maxbrunsfeld · 2017-12-14T22:50:35Z

Thanks @satchm0h! Nice work!

mdlinville · 2017-12-14T22:57:43Z

OMG! 🎉

maxbrunsfeld · 2017-12-14T23:31:49Z

This will go out in Atom 1.25.

satchm0h · 2017-12-20T03:29:56Z

Wonders never cease ;)

50Wliu · 2017-12-20T05:07:08Z

Hmm, I just identified some minor issues with this PR, specifically regarding the addition of the ;; comment character. Since you added it inside a character class, it's actually just matching ;. I'll see if I can get that fixed up in a follow-up PR.

satchm0h added 2 commits March 8, 2016 10:31

Fixing linter errors

2487ef4

50Wliu added the needs-review label Mar 8, 2016

50Wliu reviewed Mar 28, 2016
View reviewed changes

Simple refactor of 'notFirstLine' to 'firstLine' per code review comm…

a9989bb

…ents from Wliu

alflanagan mentioned this pull request Sep 19, 2017

Autoflow does not work for C89-style comment blocks #60

Closed

maxbrunsfeld suggested changes Sep 28, 2017

View reviewed changes

50Wliu added requires-changes under-review and removed needs-review labels Nov 15, 2017

- Fixing issues identified by @maxbrunsfeld code review

b9b10ab

maxbrunsfeld merged commit ab79933 into atom:master Dec 14, 2017

This was referenced Dec 16, 2017

Markdown lists are reflowed incorrectly #4

Closed

Treat '%' as a comment character #36

Closed

reflow mail-messages with '>' #39

Closed

Reflow breaks code in Lisp and Lisp-derived languages when reflowing comments #43

Closed

50Wliu removed the requires-changes label Dec 20, 2017

50Wliu mentioned this pull request Dec 21, 2017

Fix linePrefix regex bugs #64

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix up the reflow behavior of MD lists, a bunch of comment styles, and ascii email inclusions #45

Fix up the reflow behavior of MD lists, a bunch of comment styles, and ascii email inclusions #45

satchm0h commented Mar 8, 2016

satchm0h commented Mar 18, 2016

50Wliu commented Mar 28, 2016

50Wliu Mar 28, 2016

satchm0h commented May 5, 2016

mturilli commented Oct 28, 2016

mdlinville commented Nov 16, 2016

50Wliu commented Nov 16, 2016

alflanagan commented Sep 19, 2017 •

edited

Loading

satchm0h commented Sep 27, 2017 via email

maxbrunsfeld left a comment

maxbrunsfeld Sep 28, 2017 •

edited

Loading

maxbrunsfeld Sep 28, 2017

50Wliu commented Nov 15, 2017

maxbrunsfeld commented Dec 14, 2017

mdlinville commented Dec 14, 2017

maxbrunsfeld commented Dec 14, 2017

satchm0h commented Dec 20, 2017

50Wliu commented Dec 20, 2017

Fix up the reflow behavior of MD lists, a bunch of comment styles, and ascii email inclusions #45

Fix up the reflow behavior of MD lists, a bunch of comment styles, and ascii email inclusions #45

Conversation

satchm0h commented Mar 8, 2016

satchm0h commented Mar 18, 2016

50Wliu commented Mar 28, 2016

50Wliu Mar 28, 2016

Choose a reason for hiding this comment

satchm0h commented May 5, 2016

mturilli commented Oct 28, 2016

mdlinville commented Nov 16, 2016

50Wliu commented Nov 16, 2016

alflanagan commented Sep 19, 2017 • edited Loading

satchm0h commented Sep 27, 2017 via email

maxbrunsfeld left a comment

Choose a reason for hiding this comment

maxbrunsfeld Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

maxbrunsfeld Sep 28, 2017

Choose a reason for hiding this comment

50Wliu commented Nov 15, 2017

maxbrunsfeld commented Dec 14, 2017

mdlinville commented Dec 14, 2017

maxbrunsfeld commented Dec 14, 2017

satchm0h commented Dec 20, 2017

50Wliu commented Dec 20, 2017

alflanagan commented Sep 19, 2017 •

edited

Loading

maxbrunsfeld Sep 28, 2017 •

edited

Loading