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

Reorder constructors in ‘Effective Dart: Usage’ and ’Constructors’ examples #5452

Closed
wants to merge 3 commits into from

Conversation

Turskyi
Copy link

@Turskyi Turskyi commented Jan 7, 2024

In this commit, I have reordered the constructors in all examples on the ‘Effective Dart: Usage’ (https://dart.dev/effective-dart/usage) and ‘Constructors‘ (https://dart.dev/language/constructors) pages to align with the Dart and Flutter style guides.

Specifically, I followed the guidelines from ‘sort_constructors_first’ (https://dart.dev/tools/linter-rules/sort_constructors_first), ‘sort_unnamed_constructors_first’ (https://dart.dev/tools/linter-rules/sort_unnamed_constructors_first), ‘Constructors come first in a class’ (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#constructors-come-first-in-a-class) and ‘Order other class members in a way that makes sense‘ (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense).

By placing constructors before other class members, these examples now adhere to the recommended style and provide clearer, more consistent code for Dart and Flutter developers.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
  • This PR doesn’t contain automatically generated corrections or text (Grammarly, LLMs, and similar).
  • This PR follows the Google Developer Documentation Style Guidelines — for example, it doesn’t use i.e. or e.g., and it avoids I and we (first person).
  • This PR uses semantic line breaks of 80 characters or fewer.
Contribution guidelines:
  • See our contributor guide for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Code changes should generally follow the Dart style guide and use dart format.
  • Updates to code excerpts indicated by <?code-excerpt need to be updated in their source .dart file as well.

In this commit, I have reordered the constructors in all examples on the ‘Effective Dart: Usage’ page to align with the Dart and Flutter style guides. 
Specifically, I followed the guidelines from ‘sort_constructors_first’ (https://dart.dev/tools/linter-rules/sort_constructors_first), ‘sort_unnamed_constructors_first’ (https://dart.dev/tools/linter-rules/sort_unnamed_constructors_first), ‘Constructors come first in a class’ (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#constructors-come-first-in-a-class) and `Order other class members in a way that makes sense` (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense). 
By placing constructors before other class members, these examples now adhere to the recommended style and provide clearer, more consistent code for Dart and Flutter developers.
Copy link

google-cla bot commented Jan 7, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

…om ‘sort_constructors_first’ (https://dart.dev/tools/linter-rules/sort_constructors_first), ‘sort_unnamed_constructors_first’ (https://dart.dev/tools/linter-rules/sort_unnamed_constructors_first), ‘Constructors come first in a class’ (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#constructors-come-first-in-a-class) and Order other class members in a way that makes sense (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense).

Reorganized field declarations to improve readability and adhere to Dart
language conventions. This change enhances the consistency across examples for
both 'good' and 'bad' usage cases, clarifying the correct use of field
initialization.
No functional changes; this refactoring solely aims at
improving the codebase maintainability and educative quality.

Signed-off-by: Dmytro Turskyi <[email protected]>
Adopted best practices for consistent placement of constructors, following the guidelines from ‘sort_constructors_first’ (https://dart.dev/tools/linter-rules/sort_constructors_first), ‘sort_unnamed_constructors_first’ (https://dart.dev/tools/linter-rules/sort_unnamed_constructors_first), ‘Constructors come first in a class’ (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#constructors-come-first-in-a-class) and Order other class members in a way that makes sense (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense).

No functional impact, purely organizational changes to
enhance clarity.

Signed-off-by: Dmytro Turskyi <[email protected]>
@Turskyi Turskyi changed the title Reorder constructors in ‘Effective Dart: Usage’ examples Reorder constructors in ‘Effective Dart: Usage’ and ’Constructors’ examples Jan 7, 2024
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Hi @Turskyi, thanks for opening the pull request!

This is a bit challenging as this is one situation in which Dart traditionally has a different style than Flutter, where Dart developers often place fields (instance variables) before constructors.

This results in some clarity about the properties of a class but more importantly is likely more familiar to users of other related programming languages.

If you think there is discussion worth having on changing this standard though, I'd consider opening an issue on this repo with your thoughts so feedback can be collected.

Thanks again and sorry it's not so clear cut! Let me know what you think :)

@Turskyi
Copy link
Author

Turskyi commented Jan 10, 2024

Hello @parlough, thank you for your thoughtful response to my pull request!

I appreciate your perspective on the tradition of placing fields before constructors in Dart. It’s interesting to learn about these nuances and how they can vary between Dart and Flutter.

I understand that this tradition might be more familiar to developers from other programming languages. However, as Dart is often used in conjunction with Flutter, I believe it’s important to consider the impact of these stylistic differences on the developer experience.

Switching between different styles when working on Dart and Flutter projects could potentially lead to confusion. Therefore, I proposed aligning the examples in the Dart documentation with the official Dart and Flutter style guides, specifically the ‘sort_constructors_first’ rule.

I’m curious to learn more about the origins of this tradition and its continued support, especially given the existence of the official Dart linter rule (https://dart.dev/tools/linter-rules/sort_constructors_first) that recommends placing constructors first. Is there a specific reason why this tradition is favoured over the official linter rule in the Dart community?

Regarding your comment about the placement of fields providing clarity about the properties of a class, I’d like to share an example from Java:

public class Book {
    private String title;

    public Book(String title) {
        this.title = title;
    }

    public String getTitle() {
        return title;
    }

    public void setTitle(String title) {
        this.title = title;
    }
}

In this Java class, the getter and setter methods for the ‘title’ field are placed after the constructor. A similar structure can be seen in Dart:

class Book {
  Book(this.title);

  String title;
}

In this Dart class, ‘title’ is essentially a getter and is placed after the constructor. This structure aligns with the style seen in other programming languages and could potentially make the code more intuitive for developers.

I noticed that there have been discussions and issues opened regarding the ordering of class members in Dart:

  1. "Improved ordering support for class members" Improved ordering support for class members Dart-Code/Dart-Code#4657
  2. "Enforce consistent order of class members" Enforce consistent order of class members linter#2367

These discussions highlight the importance of consistency in code structure. In my opinion, maintaining a consistent order, even if it’s not perfect, could be more beneficial than having multiple good “traditions” within the same code base.

If you believe that further discussion is needed, I’m open to creating another issue to continue this conversation.

Thank you again for your time and consideration. 😊

@atsansone
Copy link
Contributor

This work was duplicated in #5718

@atsansone atsansone added st.blocked Issue cannot continue until another action completes review.tech Awaiting Technical Review labels Apr 18, 2024
@mit-mit mit-mit requested a review from munificent April 19, 2024 09:00
@atsansone
Copy link
Contributor

Closing this issue as Won't fix.

@atsansone atsansone closed this Apr 19, 2024
@Turskyi
Copy link
Author

Turskyi commented Apr 19, 2024

Hello @atsansone,

I've noticed that the pull request I submitted was closed with the status "Won't fix." I understand that not all contributions may align with the project's direction or current priorities. However, I observed that the pull request was still awaiting a review from @munificent, as indicated in the "reviewers" section.

Given that @parlough, despite having a different view, did not close the pull request, I'm curious about the protocol followed in this situation. Could you please provide some insight into the reasons behind the decision to close the request without a review from all assigned reviewers?

Understanding the rationale behind this process would be greatly beneficial for my future contributions and for ensuring they are in line with the project's goals. It would also help clarify the expectations for all contributors regarding the review process.

Thank you for your time and consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.tech Awaiting Technical Review st.blocked Issue cannot continue until another action completes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants