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

fix(SNP-1286): rules member ordering #13

Merged
merged 9 commits into from
May 28, 2024

Conversation

ilyasov-ainur
Copy link
Contributor

Changes

  • Changed member-ordering

  • Changed arguments-ordering.
    Added last order for :
    - body
    - child
    - children

  • Added parameters-ordering

@ilyasov-ainur ilyasov-ainur added the WIP Work in process label Apr 23, 2024
Copy link

Link to Pyrus task

lib/dart.yaml Outdated
Comment on lines 4 to 8
- arguments-ordering:
last:
- body
- child
- children
Copy link
Contributor Author

@ilyasov-ainur ilyasov-ainur Apr 24, 2024

Choose a reason for hiding this comment

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

This order of arguments is needed for the child, children and body arguments come last.

Note: Text styles use the body style. Should I add an excluded to it?

Choose a reason for hiding this comment

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

Note: Text styles use the body style. Should I add an excluded to it?

You mean there is a body text style in theme extensions?

You can look at ignoring the rule for files that describe styles, as a rule they have theme and extension in the filename, which can be used as a reference. But in this case you will need to fix this nuance of the linter either in the template project (to maintain a unified projects structure), or for the template project add an override of this rule in analysis rule file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I meant text extensions.
At first, I added excluding for text_scheme file, but then i removed it in this commit
because I also thought it would be better to do it in a template project

lib/dart.yaml Outdated Show resolved Hide resolved
lib/dart.yaml Outdated Show resolved Hide resolved
@ilyasov-ainur ilyasov-ainur self-assigned this Apr 24, 2024
@ilyasov-ainur ilyasov-ainur added bug Something isn't working and removed WIP Work in process bug Something isn't working labels Apr 24, 2024
@ilyasov-ainur ilyasov-ainur changed the title fix(SNP-1286): rules member ordering #12 fix(SNP-1286): rules member ordering Apr 24, 2024
lib/dart.yaml Outdated
Comment on lines 9 to 12
- parameters-ordering:
required: "first"
default: "last"
super: "last"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This order of parameters inside class constructors.

Note: I didn't find:

  1. how to disable alphabetical order;
  2. how to set the order of anonymous fields that are not referred to class fields
    Example:
ElementaryWidget({
    super.key,
    /// For this parameter
    WidgetModelFactory wmFactory = defaultWMFactory,
})

Choose a reason for hiding this comment

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

  1. how to disable alphabetical order;
  2. how to set the order of anonymous fields that are not referred to class fields
  1. I think alphabetical sorting is OK, it's easier to navigate the code that way.
  2. Isn't "default" option responsible for this? https://dcm.dev/docs/rules/common/parameters-ordering/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes, but in our projects such parameters often come after super, and parameters with default values, which are specified through this, come before.

Choose a reason for hiding this comment

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

  1. Yes, but in our projects such parameters often come after super, and parameters with default values, which are specified through this, come before.

Does this only apply to ElementaryWidget, or are there other cases? In the case of ElementaryWidget I tend to think that such widgets should not have their own final fields, the arguments should be passed directly to WM

Choose a reason for hiding this comment

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

I think alphabetical sorting is OK, it's easier to navigate the code that way.

To be honest, I used to order arguments by semantic. E.g.:

  const SomeWidget({
    required this.id,
    required this.title,
    required this.description,
    required this.closeIcon,
    super.key,
  });

With alphabetic order it'll be look like this:

const SomeWidget({
      required this.closeIcon,
      required this.description,
      required this.id,
      required this.title,
      super.key,
    });

This is not a big problem, but it can be inconvient in some cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I used to order arguments by semantic. E.g.:

I agree, that's why I wanted to turn off the alphabetical order

Copy link
Contributor Author

@ilyasov-ainur ilyasov-ainur Apr 25, 2024

Choose a reason for hiding this comment

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

Does this only apply to ElementaryWidget, or are there other cases?

I can't think of a practical example using all types of parameters...

Example:

class SomeSuperClass {
  final int? a;

  const SomeSuperClass({this.a});
}

class SomeClass extends SomeSuperClass {
  final int b;
  final int d;
  final int _c;

  SomeClass({
    required this.d,
    super.a,
    this.b = 1,
    int c = 2,
  }) : _c = c;
}

Copy link

@plasticfiresam plasticfiresam left a comment

Choose a reason for hiding this comment

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

Let`s finish the body-text issue and we can merge, LGTM

lib/dart.yaml Outdated
Comment on lines 9 to 12
- parameters-ordering:
required: "first"
default: "last"
super: "last"

Choose a reason for hiding this comment

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

  1. how to disable alphabetical order;
  2. how to set the order of anonymous fields that are not referred to class fields
  1. I think alphabetical sorting is OK, it's easier to navigate the code that way.
  2. Isn't "default" option responsible for this? https://dcm.dev/docs/rules/common/parameters-ordering/

@plasticfiresam
Copy link

@Sadhorsephile
Can you participate in the review too?

lib/dart.yaml Outdated
Comment on lines 9 to 12
- parameters-ordering:
required: "first"
default: "last"
super: "last"

Choose a reason for hiding this comment

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

I think alphabetical sorting is OK, it's easier to navigate the code that way.

To be honest, I used to order arguments by semantic. E.g.:

  const SomeWidget({
    required this.id,
    required this.title,
    required this.description,
    required this.closeIcon,
    super.key,
  });

With alphabetic order it'll be look like this:

const SomeWidget({
      required this.closeIcon,
      required this.description,
      required this.id,
      required this.title,
      super.key,
    });

This is not a big problem, but it can be inconvient in some cases

lib/dart.yaml Outdated Show resolved Hide resolved
lib/dart.yaml Outdated Show resolved Hide resolved
lib/dart.yaml Outdated Show resolved Hide resolved
lib/dart.yaml Outdated Show resolved Hide resolved
lib/dart.yaml Outdated Show resolved Hide resolved
@dzmitry-struk-surf dzmitry-struk-surf merged commit 98d8e4d into main May 28, 2024
1 check passed
@dzmitry-struk-surf dzmitry-struk-surf deleted the SNP-1286-rules-member-ordering branch May 28, 2024 06:45
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.

4 participants