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

remove leading and trailing empty lines in a block #311

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

Conversation

flan6
Copy link

@flan6 flan6 commented Aug 23, 2024

This pull request is intended to achieve what was discussed on issue #284 leaving empty trailing lines in a block if it is followed by an else-if statement, otherwise removes it. Also takes over (somewhat) PR #286.

@mvdan please feel free to suggest any change you see suitable.

close #284

ensures that format removes empty lines at the top of a block unconditionally and bottom unless it is followed by an else-if statement.
see issue mvdan#284
ensures that empty line before an else-if statement is not removed.
see mvdan#284.
see mvdan#284.
trailing empty lines are removed only if it is not followed by an
else-if statement.
@bshelton
Copy link

bshelton commented Nov 8, 2024

LGTM, what are we waiting on here?

@mvdan
Copy link
Owner

mvdan commented Nov 13, 2024

Thanks for the patch and apologies for the slow response.

The code change looks fine. I also ran the updated gofumpt on a number of my projects and it mostly looks good, except in two cases.

First, if-elses still remove the trailing empty line, because your code only opts out for if-elseif. I think that needs to be adjusted so that the empty line below is not removed:

    meta := s.MetadataGraph()
    var rdeps map[PackageID]*metadata.Package
    if transitive {
        rdeps = meta.ReverseReflexiveTransitiveClosure(id)
 
        // Remove the original package ID from the map.
        // (Callers all want irreflexivity but it's easier
        // to compute reflexively then subtract.)
        delete(rdeps, id)
-
    } else {
        // direct reverse dependencies
        rdeps = make(map[PackageID]*metadata.Package)
        for _, rdepID := range meta.ImportedBy[id] {
            if rdep := meta.Packages[rdepID]; rdep != nil {
                rdeps[rdepID] = rdep
            }
        }
    }

Second, I wonder whether this change is a good idea or not:

    switch tok {
-
    case token.FOR, token.IF:
        if rhs {
            expr = p.parseExpr()
            break
        }
        comp, ident := p.parseComprehension()
        if comp != nil {
            return nil, nil, comp, false
        }
        expr = ident
 
    case token.LET:
        let, ident := p.parseLetDecl()
        if let != nil {
            return nil, nil, let, false
        }
        expr = ident

Note how empty lines are used to separate the switch cases, so a leading empty line does add some symettry. Given how common large switches with many cases are, perhaps we should always allow a leading empty line in a switch as a special case. I don't think it's worth getting into heuristics for them.

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.

Remove empty lines in if and for similar to functions
3 participants