Skip to content

feat(eslint-plugin): [member-ordering] add support for grouping readonly fields #6349

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 13, 2023
Merged

feat(eslint-plugin): [member-ordering] add support for grouping readonly fields #6349

merged 6 commits into from
Mar 13, 2023
+729 −14

Conversation

IronGeek
Copy link
Contributor

PR Checklist

Overview

This PR introduces new member attribute/type/Kind: readonly-field, a superset of field, which denotes all fields with readonly modifiers. This makes it possible to group/ sort fields based on their readonly modifiers and optionally with their Accessibility, and or Scope too.

Like other member types, readonly-field can also be joined with accessibility and scope attributes to create more specific type; the following is the complete combination of readonly-field with Scope and Accessibility introduced in this PR:

"readonly-field"
"public-readonly-field"
"public-decorated-readonly-field"
"decorated-readonly-field"
"static-readonly-field"
"public-static-readonly-field"
"instance-readonly-field"
"public-instance-readonly-field"
"abstract-readonly-field"
"public-abstract-readonly-field"
"protected-readonly-field"
"protected-decorated-readonly-field"
"protected-static-readonly-field"
"protected-instance-readonly-field"
"protected-abstract-readonly-field"
"private-readonly-field"
"private-decorated-readonly-field"
"private-static-readonly-field"
"private-instance-readonly-field"
"#private-readonly-field"
"#private-static-readonly-field"
"#private-instance-readonly-field"

For example, with this configuration the following code would be treated as valid:

{
  "rules": {
    "@typescript-eslint/member-ordering": [
      "error", { 
        "default": [
            "decorated-readonly-field",
            ["public-readonly-field", "readonly-field"]
            "protected-readonly-field",
            "private-readonly-field",
            "abstract-readonly-field",
            "field"
        ] 
      }
    ]
  }
}
abstract class Foo {
  @Dec public readonly DA: string;
  @Dec protected readonly DB: string;
  @Dec private readonly DC: string;

  public static readonly SA: string;
  public readonly IA: string;
  static readonly #SD: string;
  readonly #ID: string;

  protected static readonly SB: string;
  protected readonly IB: string;

  private static readonly SC: string;
  private readonly IC: string;

  public abstract readonly AA: string;
  protected abstract readonly AB: string;

  public RA: string;
  RB: string;
  private RC: string;
  #RD: string;
}

@nx-cloud
Copy link

nx-cloud bot commented Jan 18, 2023 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 534ac14. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

🟥 Failed Commands
Node 14 - nx test typescript-estree --coverage=false
✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @IronGeek!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Jan 18, 2023 •
edited
Loading

✅ Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 534ac14
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/640ea9bbfa9041000889496e
😎 Deploy Preview https://deploy-preview-6349--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Jan 18, 2023 •
edited
Loading

Codecov Report

Merging #6349 (9f2e32c) into main (36ef0e1) will increase coverage by 2.65%.
The diff coverage is 100.00%.

❗ Current head 9f2e32c differs from pull request most recent head 534ac14. Consider uploading reports for the commit 534ac14 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6349      +/-   ##
==========================================
+ Coverage   88.62%   91.28%   +2.65%     
==========================================
  Files         382      366      -16     
  Lines       12883    12461     -422     
  Branches     3787     3656     -131     
==========================================
- Hits        11418    11375      -43     
+ Misses       1124      772     -352     
+ Partials      341      314      -27
Flag Coverage Δ
unittest 91.28% <100.00%> (+2.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ackages/eslint-plugin/src/rules/member-ordering.ts 96.41% <100.00%> (+0.27%) ⬆️

... and 56 files with indirect coverage changes

JoshuaKGoldberg
JoshuaKGoldberg requested changes Jan 22, 2023
View reviewed changes
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

A good start - thanks for getting this PR up! 🙌

Requesting changes on more test coverage & handling index signatures.

Aside: this rule gets more complicated and hard to deal with in every PR. But I don't see a way around it 😞. This comment isn't actionable and you can ignore my sadness. I'm just ... musing.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 22, 2023
@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 13, 2023
JoshuaKGoldberg
JoshuaKGoldberg approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Whew, looks good to me! Thanks @IronGeek! 🙌

In theory one might want something like readonly-method, for cases like { readonly A: () => void }. But that can be a feature add if requested after this PR. The rule is (and already was) complex enough as it is.

@JoshuaKGoldberg
Copy link
Member

Btw, sorry for the month-long wait time. This fell off my radar. After the v6 release I'm hopeful we can work on #6033 as a process improvement so it doesn't happen again.

@bradzacher bradzacher enabled auto-merge March 13, 2023 05:00
@JoshuaKGoldberg JoshuaKGoldberg merged commit 9d3bdfc into typescript-eslint:main Mar 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[member-ordering] separating readonly members is not supported
2 participants
@IronGeek @JoshuaKGoldberg