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

Force lines before and after some members. #285

Closed
GravlLift opened this issue Jun 11, 2021 · 8 comments · Fixed by #535
Closed

Force lines before and after some members. #285

GravlLift opened this issue Jun 11, 2021 · 8 comments · Fixed by #535

Comments

@GravlLift
Copy link

GravlLift commented Jun 11, 2021

Csharpier likes getting rid of duplicate lines, but doesn't seem to have an opinion on single line breaks between methods. Both of these examples are unaltered by running csharpier, for example:

public string Foo() 
{
  return "foo";
}

public string Bar() 
{
  return "bar";
}
public string Foo() 
{
  return "foo";
}
public string Bar() 
{
  return "bar";
}

Personally the first one is more appealing to me, but either way, an opinion would be useful.

Are there any plans to implement something like this? I assume there a whole mess of edge cases I'm not considering here.

@respel
Copy link

respel commented Jun 11, 2021

Thanks for the issue, Jason.

The reason we don't do it right now is that it's tricky. CSharpier is heavily inspired by prettier and follows prettier's rationale regarding empty lines.

I agree with you though that we should consider it at some point of time.

Are there any plans to implement something like this?

Not that I know of. This is imo the first issue that we've had regarding this.

@respel
Copy link

respel commented Jun 11, 2021

@belav @shocklateboy92 How do you guys feel about this?

@GravlLift GravlLift reopened this Jun 11, 2021
@belav
Copy link
Owner

belav commented Jun 11, 2021

It is definitely something we should consider.

I personally like to have a blank line between methods. Unless the methods are expression bodied, then no lines. Usually no lines between properties. But if properties have attributes then I like a line between them. Etc. My point being that if we go down this path, we'd have to decide on the rules for when csharpier should insert a blank line.

The alternative would be to use something else like stylecop to enforce the rules for where blank lines are, and let csharpier format everything around it. There are a number of stylecop rules that conflict with csharpier though, so we need to determine what those are so we can provide people with the list of rules to disable when using csharpier.

I'm not sure that I have a strong opinion on if Csharpier should add empty lines or not. I lean towards no, but that's mostly because I know there is a ton of other work to get done.

@shocklateboy92
Copy link
Collaborator

shocklateboy92 commented Jun 12, 2021

I just read the doc that @respel linked, and I don't think it necessarily applies here. As far as I can tell, it's referring to the fact that prettier no longer removes blank lines (I remember it used to, in their earlier versions). I don't think they have many qualms with adding them. (I swear I've come across examples where they do so).

That being said, I too don't have particularly strong opinions one way or the other. @GravlLift feel free to make a pull request if you do 😄

@belav
Copy link
Owner

belav commented Jun 12, 2021

Prettier for ts/js definitely removes blank lines if you have multiples in a row. I'm not sure about if it ever adds them.

The prettier-java plugin does add them in some cases.
This PR is to enforce adding a blank line at the start of classes - jhipster/prettier-java#398
And there is some discussion about other places to add lines, although it doesn't seem to have gone anywhere yet - jhipster/prettier-java#284

The prettier philosophy of adding/removing blank lines may very from plugin to plugin. Maybe the official plugins are more consistent with the philosophy.

@shocklateboy92
Copy link
Collaborator

The markdown plugin adds a ton of newlines 🤿

@Kurt-von-Laven
Copy link
Contributor

Black does a really good job of handling (both adding and removing) empty lines in my opinion if someone decides to take this on some day.

@belav
Copy link
Owner

belav commented Dec 31, 2021

This is the current set of rules I am considering implementing.

I was considering removing lines between fields and single line properties, but digging through public repos it seems pretty common for people to group fields and properties using a blank line between the groups.

public class ClassRules
{
    public void NeverLineBeforeFirstMember() { }

    public void AlwaysLineBetweenMethods() { }

    public void EvenWithLambdaBody() => "";

    private enum AlwaysLineBeforeEnum
    {
        One,
        Two
    }

    private class AlwaysLineBeforeClass { }

    private interface IAlwaysLineBeforeInterface { }

    // the rest of these also force a line
    // constructor
    // destructor
    // conversion operator
    // record declaration
    // struct declaration
    // operator

    private string FieldsCanBeGroupedBecauseLinesWillBeKept;
    private string ButLinesAreNotForcedHere;

    // comments force a line
    private string FieldWithComment;

    /* comments force a line */
    private string FieldWithComment;

    /// <summary> comments force a line </summary> ///
    private string FieldWithComment;
    
    [AttributesForceALine]
    private string FieldWithAttribute;

    // treat the rest of these the same as fields
    // properties
    // indexers
    // event declarations
    // event fields
    // delegate declarations
}

public interface InterfaceRulesAreLikeClassesButMethodsFollowTheFieldRulesAbove
{
    void NeverLineHere();
    
    void ExistingLineHereIsKept();
    void ButNoLineIsAdded();
    
    // comments do force a line
    void MethodWithComment();
}

// records and structs should be the same as class rules

public enum EnumRules
{
    ALineAboveThisWouldBeRemoved,
    ALineAboveThisWouldBeRemovedAsWell,
    
    // unless there is a comment, then force one,
   LineHere,

   [AttributesAlsoForceLines]
   LineHere,
}

@belav belav closed this as completed in #535 Jan 3, 2022
belav added a commit that referenced this issue Jan 3, 2022
* Starting to implement rules for when to force a line before some members

closes #285

* Mostly finished with logic for forcing lines.

* Self code review

* Fixing an edge case with directives

* Adding more edge cases that need to be fixed.

* Fixing some more edge cases with preprocessor symbols

* More edge cases that need to be resolved

* More edge cases

* Another edge case

* More edge cases

* Even more edge cases

* Maybe the last edge case

* Fixing test

* Self code review
@belav belav changed the title Require Lines Between Methods? Force lines before and after some members. Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants