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

Handle block of blank lines for code folding properly (fixes #1707). #1756

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

torimus
Copy link

@torimus torimus commented Aug 11, 2021

Consider context of consequent blank lines, to which code & indentation level it belongs.
Leaves blank lines of outer indentation levels intact.

Fixes issue #1707.

Copy link
Member

@purcell purcell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Looks reasonable - I noted a couple of minor issues.

I wonder what you think of my suggestion in the linked issue that we retain a single blank line?

@@ -46,6 +46,19 @@
(= (point-at-eol)
(progn (skip-chars-forward "[:blank:]") (point)))))

(defun haskell-blank-lines-block (cmp dir indent)
"returns `t' if following sequence of blank lines fits specific indentation level"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick fix to follow docstring conventions:

Suggested change
"returns `t' if following sequence of blank lines fits specific indentation level"
"Return non-nil if sequence of blank lines in direction DIR fits given INDENT level."

(haskell-blank-line-p)))
(if (funcall cmp (current-indentation) indent)
(progn
(when (= dir 1) (forward-line -1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use spaces for indents.

(progn
(when (= dir 1) (forward-line -1))
t)
(progn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This progn wrapper is redundant in the else clause of if statements, and it would be more idiomatic to omit it.

@torimus
Copy link
Author

torimus commented Aug 12, 2021

@purcell

A quick fix to follow docstring conventions

Thanks for the correction, fixed in a new commit.

Please use spaces for indents

Oh sorry, wasn't aware emacs doesn't automatically sets spaces-only for elisp mode indentation like used in other language modes. Fixed.

This progn wrapper is redundant in the else clause of if statements, and it would be more idiomatic to omit it.

Right. I'm not learned in Emacs Lisp, now found out in docs else branch is not limited to a single expression.

I wonder what you think of my suggestion in the linked issue that we retain a single blank line?

Not sure if I follow you correctly, but this PR introduces 3 new failed tests in haskell-collapse-tests.el
FAILED test-haskell-indented-block-1
FAILED test-haskell-indented-block-3
FAILED test-haskell-indented-block-7

which all depend how would you expect correct code folding. In my opinion, retained blank line(s) between code blocks at the same indentation level(as the initial) are to be expected. Only collapse them in nested indentation levels. I can "fix" tests appropriately to correspond to aforementioned logic so it will pass.

…ie. retain block of blank lines followed by same (or lesser) indentation level as the initial one.
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

Successfully merging this pull request may close these issues.

None yet

2 participants