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

Edit constructor page #5718

Closed
wants to merge 5 commits into from
Closed

Edit constructor page #5718

wants to merge 5 commits into from

Conversation

atsansone
Copy link
Contributor

Fixes #5462

@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Apr 12, 2024

Visit the preview URL for this PR (updated for commit ce21740):

https://dart-dev--pr5718-fix-5462-sn0zlxf6.web.app

Comment on lines -11 to -15
final double x;
final double y;

// Sets the x and y instance variables
// before the constructor body runs.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing from the multiple Point class examples on the page.

image

The comment is helpful and shouldn't be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-added it, but it doesn't appear anywhere that I could see after.

@@ -34,17 +34,17 @@ class Point {
The `this` keyword refers to the current instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the above example (can't comment directly on it, lines 23-32) be part of the "sort the constructor first" updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

praise: Good catch!

Comment on lines -287 to -290
Point(double x, double y)
: x = x,
y = y,
distanceFromOrigin = sqrt(x * x + y * y);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't figure this out myself, but is there a functional reason the assignments were written out like this before, instead of using this? The example is in the "Initializer list" section so maybe not using this is intentional to show a fully written out initializer list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my guess. This copy precedes a 2016 site migration, so I have no idea.

@MaryaBelanger
Copy link
Contributor

I think the conversation of whether this style needs to be enforced across all our documentation should be a conversation involving the wider team. This feels like more of a design decision than a docs decision. Yes, we have a lint sort_constructors_first but it's personal choice, not even part of the recommended set. And we don't have an effective dart entry as far as I can tell.

@atsansone atsansone added review.copy Awaiting Copy Review review.tech Awaiting Technical Review labels Apr 15, 2024
@atsansone
Copy link
Contributor Author

That's fine. Maybe see if @eernstg has an opinion?

@eernstg
Copy link
Member

eernstg commented Apr 17, 2024

an opinion?

Not really. ;-)

I don't think any particular ordering dominates the Dart software universe, and nothing is supported explicitly by the style guide or similar documents (that I know of). According to directives_ordering, imports should be sorted by kind (platform, package, relative) and then alphabetically, following Effective Dart. It probably makes sense to extrapolate this to class member declarations (and mixin member declarations, etc.) This means that we'd have all the constructors together, all static members together, etc. It's still a non-trivial decision whether each group is sorted alphabetically, or it is organized "semantically" in some sense, or even unsorted/random.

I don't think the community has agreed on much more: Perhaps constructors should go first, perhaps instance variables should go first (this seems to be quite common), and who knows whether static members should come before instance members or vice versa, and who knows whether we'd put all the instance members into one group, or keep getters and setters in one group and methods in another group (same for static, by the way). It would also be possible (and it seems consistent to me) to treat instance variables exactly like the getters/setters that they introduce, but I don't think that's common at all.

@munificent, do you think there's anything like a canonical ordering of class member declarations, or even a particular style which is more common than others?

@atsansone
Copy link
Contributor Author

FYI: I'm not invested in the ordering myself. I thought that the issue submitter had a point and made changes accordingly. If you feel that a definitive stance is premature, I'm happy to shelve this.

@atsansone atsansone added the st.blocked Issue cannot continue until another action completes label Apr 18, 2024
@natebosch
Copy link
Member

I do not think we should make any specific recommendation. When this has been discussed internally (where we have an even higher impetus for consistency) we have decided against adapting a fixed ordering. There can be strong reasons to order members in different ways based on the ways the members related to eachother.

If a class is so big that rules like this feel necessary to avoid getting lost in the source code, then it should probably be split up.

The flutter repo does specify this, but the flutter repo style guide is not written with the intention of being a recommendation for a wider audience like the package:lints rule sets or Effective Dart.

@MaryaBelanger
Copy link
Contributor

@atsansone Let's close this PR and #5452 based on Nate's comment (and Parker's older one). I don't think it's meaningful to standardize for Dart, and I strongly agree with Parker's point:

This [Dart's tendency to place field declarations first despite Flutter style] results in some clarity about the properties of a class but more importantly is likely more familiar to users of other related programming languages.

@atsansone
Copy link
Contributor Author

Closing this issue as Won't fix.

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

Turskyi commented Apr 20, 2024

Hello @atsansone, @MaryaBelanger, @eernstg, and @natebosch,

I've been closely following the discussions around the Dart documentation style and have noticed the closure of my original issue #5462 and pull request #5452. I appreciate the team's consideration of various viewpoints.

I'm encouraged by the move towards greater consistency in the documentation, as seen in PR #5718, which closely aligns with the changes I proposed in #5452. Open-source thrives on transparent and inclusive collaboration, and I would have valued the opportunity to be part of the conversation, especially to understand the team's perspective more deeply.

I'd like to highlight a point from my comment about the placement of fields and constructors, supported by examples from Java and Dart. This practice is intuitive and aligns with styles in other languages, potentially benefiting developers.

Here are the examples for clarity:
In 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;
    }
}

And in Dart:

class Book {
  Book(this.title);

  String title;
}

In these examples, the practice of placing constructors before properties, such as getters and setters, is common and aligns with styles seen in other programming languages. This approach can make the code more intuitive for developers. In the Dart class, ‘title’ serves not just as a field but also implicitly as a getter and setter, which is why it is positioned after the constructor, following the idiomatic use of Dart’s syntactic sugar for more concise code.

While I respect the decision to close the issue as “Won’t fix,” I believe that my refutation of the quoted point was not fully considered in the discussion. It’s important for the community to have a comprehensive understanding of different perspectives before reaching a conclusion.

I hope that we can revisit this topic with a broader scope, perhaps involving more community members, to ensure all viewpoints are represented, leading to a more informed decision-making process.

I’m open to adapting my contributions based on the team’s feedback and the community’s consensus. I look forward to continuing to contribute to the Dart community and would appreciate any feedback on this message.

Thank you for your time.

@eernstg
Copy link
Member

eernstg commented Apr 22, 2024

I hope that we can revisit this topic

I think it will be discussed in the future. You could take a look at dart-lang/linter#2367, perhaps voting for it and/or contributing to the discussion. At this time I don't think anyone has created a set of rules which is obviously the way to go, and it doesn't make much sense to enforce a set of rules for anyone who might have good reasons to do something else (e.g., ordering instance variable declarations according to their mutual relationships and semantics). So the next step would surely be to build consensus around a particular approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.copy Awaiting Copy Review 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.

Reorder constructors and fields in code example on Constructors page
7 participants