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

Disable alphabetic class members #10

Open
miklschmidt opened this issue May 31, 2017 · 8 comments
Open

Disable alphabetic class members #10

miklschmidt opened this issue May 31, 2017 · 8 comments

Comments

@miklschmidt
Copy link
Member

Closely related to #9

Member sorting
pros:

  • a very consistent albeit arbitrary ordering of member names
  • to work around issues such as width and height grouping you can create special rules

cons:

  • You can't group keys that logically belong together, such as showModal and toggleModal.
  • If you want to achieve the previous, you're forced to write less readable (not natural language) member names, such as modalToggle, modalShow. Yoda style, a modal show you must, young padawan. This results in less readable code.
@kastermester
Copy link
Member

What is the proposed change here? Removing ordering entirely or ...? What would you like to see instead?

@miklschmidt
Copy link
Member Author

Either disabling alphabetical class member sorting or what you proposed here: #9 (comment)

@kastermester
Copy link
Member

So you still want to keep them grouped by access level and type?

I'm also game for implementing some comment based way of grouping things (within access level and type still i think?) for class members.

@miklschmidt
Copy link
Member Author

miklschmidt commented May 31, 2017

So you still want to keep them grouped by access level and type?

I don't have a strong opinion on access level, but it'd probably make sense to apply that to groups instead of the entire class? Not sure. Type is great as it is if you mean the react member ordering has it's own group like it has now.

Comment based grouping would be the tits!

@kastermester
Copy link
Member

kastermester commented May 31, 2017

By type i meant whether it is a method or a regular class member. Also there's the whole static/non-static stuff.

There's a bunch of decisions around the exact semantics here, but I'm not sure a natural fit would present itself without trying it out.

@miklschmidt
Copy link
Member Author

Ah yes, let's keep those at the top of the file, then we can always at grouping support for those later.

@kastermester
Copy link
Member

kastermester commented May 31, 2017

Here's one proposal for semantics around groups in classes (note my strong preference for keeping fields, ie. not methods, at the top of the class).

The general idea here is that the ungrouped orderings are as they are right now:

  • private-static-field
  • protected-static-field
  • public-static-field
  • private-static-method
  • protected-static-method
  • public-static-method
  • private-instance-field
  • protected-instance-field
  • public-instance-field
  • constructor
  • react-lifecycle-method
  • private-instance-method
  • protected-instance-method
  • public-instance-method
  • react-render-method

Here the change is to allow groups to be "shoved" in at certain points in this list

  • private-static-field
  • protected-static-field
  • public-static-field
    • Grouped static fields, groups are alphabetical ordered and the members are in the order specified above.
  • private-static-method
  • protected-static-method
  • public-static-method
    • Grouped static methods, groups are alphabetical ordered and the methods are in the order specified above.
  • private-instance-field
  • protected-instance-field
  • public-instance-field
    • Grouped instance fields, groups are alphabetical ordered and the members are in the order specified above.
  • constructor
  • react-lifecycle-method
  • private-instance-method
  • protected-instance-method
  • public-instance-method
    • Grouped instance methods, groups are alphabetical ordered and the members are in the order specified above.
  • react-render-method

Here's an example of valid layout

class MyClass {
  private static myPrivateStaticField: string;
  protected static myProtectedStaticField: string;
  public static myPublicStaticField: string;

  // MY GROUP
  private static myGroupedPrivateStaticField: string;
  protected static myGroupedProtectedStaticField: string;
  public static myGroupedPublicStaticField: string;

  private static myPrivateStaticMethod(): string {
    return 'Hello';
  }
  protected static myProtectedStaticMethod(): string {
    return 'Hello';
  }
  public static myPublicStaticMethod(): string {
    return 'Hello';
  }

  // MY GROUP
  private static myGroupedPrivateStaticMethod(): string {
    return 'Hello';
  }
  protected static myGroupedProtectedStaticMethod(): string {
    return 'Hello';
  }
  public static myGroupedPublicStaticMethod(): string {
    return 'Hello';
  }

  private myPrivateInstanceField: string;
  protected myProtectedInstanceField: string;
  public myPublicInstanceField: string;

  // MY GROUP
  private myGroupedPrivateInstanceField: string;
  protected myGroupedProtectedInstanceField: string;
  public myGroupedPublicInstanceField: string;

  public constructor() {
  }

  // React life cycle methods can go here, they cannot be a part of a group

  private myPrivateInstanceMethod(): string {
    return 'Hello';
  }
  protected myProtectedInstanceMethod(): string {
    return 'Hello';
  }
  public myPublicInstanceMethod(): string {
    return 'Hello';
  }

  // MY GROUP
  private myGroupedPrivateInstanceMethod(): string {
    return 'Hello';
  }
  protected myGroupedProtectedInstanceMethod(): string {
    return 'Hello';
  }
  public myGroupedPublicInstanceMethod(): string {
    return 'Hello';
  }

  // React render method can go here, it cannot be a part of a group
}

There's plenty other options here - and it is quite hard to determine where the boundaries of groups should be. Other ways of doing this are more than welcome.

@miklschmidt
Copy link
Member Author

I like it!

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

No branches or pull requests

2 participants