-
Notifications
You must be signed in to change notification settings - Fork 716
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
Update Effective Dart Design Guidelines to Reflect Dart 3.0 Class Modifiers #6449
Conversation
1. Renamed the variable json to data throughout the section. 2. Updated the text to clarify that the example works with deserialized data (e.g., parsed from JSON) rather than raw JSON strings. 3. Ensured consistency in variable naming and explanations. This update makes the documentation more accurate and avoids misleading readers into thinking that the variable holds a JSON string. If you’re contributing to the Dart documentation, you can submit this change as a pull request to the [site-www repository](https://github.com/dart-lang/site-www)
Update the 'Classes and mixins' section to reflect Dart 3.0’s class modifier features: - Revise API maintainer guidelines to recommend inal, �ase, etc., over documentation for controlling class extension and implementation. - Remove obsolete user guidelines ('AVOID extending/implementing') as compiler enforcement via modifiers replaces the need for these. - Add a note explaining the shift and obsoletion, placed at the section’s start for context. This aligns the guidelines with modern Dart language capabilities, reducing reliance on docs.
/gcbrun |
Visit the preview URL for this PR (updated for commit a57c770): https://dart-dev--pr6449-guidelines-update-v1-mjkxucz1.web.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few notes added plus it looks like you need to decouple some JSON changes.
src/content/effective-dart/design.md
Outdated
classes. But Dart does not require all code to be defined inside a | ||
class—you can define top-level variables, constants, and functions like | ||
you can in a procedural or functional language. | ||
**Note:** As of Dart 3.0, class modifiers (e.g., `final`, `base`, `interface`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only include very basic top-level information under this high-level heading. Let's revert this to what was already there, but I would advise adding one bullet with a sentence or two that lets people know that classes can be restricted.
I believe you've already captured these other details in subsections. If something is here but not in one of those sections, let's move the content to that section.
src/content/effective-dart/design.md
Outdated
|
||
### DO document if your class supports being used as an interface | ||
### DO use class modifiers to control if your class can be used as an interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### DO use class modifiers to control if your class can be used as an interface | |
### DO use class modifiers to control if your class can be an interface |
I think we can cut a few words from this without removing meaning.
src/content/effective-dart/design.md
Outdated
to implement, then changing those classes becomes very difficult. That | ||
difficulty in turn means the libraries you rely on are slower to grow and adapt | ||
to new needs. | ||
Since Dart 3.0, class modifiers like `final`, `base`, or `sealed` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Dart 3.0, class modifiers like `final`, `base`, or `sealed` | |
Class modifiers like `final`, `base`, or `sealed` |
Dart 3 has been out for a while. I think we can avoid mentioning it. @johnpryan, your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - we can leave this out here.
src/content/effective-dart/design.md
Outdated
|
||
If your class can be used as an interface, mention that in the class's doc | ||
comment. | ||
With Dart 3.0’s class modifiers, you can restrict implementation using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you have this section mirror the last one as closely as possible? It's okay if they sound very similar. I like the wording you used in the previous section for extending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see several changes in here related to JSON data. I suspect these are not part of the class modifiers PR. Can you update your PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The json changes are of another PR
but this PR doesn't contain any of those changes
this one is created on a separate branch
idk why its showing like that :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the conflict warning has been taken care of :)
a false indentation in 'src/content/language/patterns.md' has been creating the problem
thank you
1)Reverted back the top level information 2)Shortend the wordings as mentioned 3)Rephrased the lines 4)Mirrored the mentioned section as said
902ea69
to
e3db6c1
Compare
@antfitch i made the mentioned changes as said |
src/content/effective-dart/design.md
Outdated
allow you to enforce whether a class can be extended directly in code. | ||
For example, use `final class A {}` to prevent extension, | ||
or `base class B {}` to allow extension only within the same library. | ||
Rely on these modifiers rather than documentation to communicate and | ||
enforce your intent. | ||
|
||
|
||
### DO use class modifiers to control if your class can be used as an interface | ||
### ### DO use class modifiers to control if your class can be an interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete extra ###
src/content/effective-dart/design.md
Outdated
`final`, `base`, or `interface`. For example, `final class A {}` | ||
prevents implementation, while `interface class C {}` allows it explicitly. | ||
Use these modifiers to enforce your design intent instead of relying solely | ||
you can also restrict implementation using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can restrict implementation using
src/content/effective-dart/design.md
Outdated
- For example, `final class A {}` prevents implementation, while `interface class C {}` allows it explicitly. | ||
- Use these modifiers to enforce your design intent instead of relying solely on documentation. | ||
|
||
Classes. But Dart does not require all code to be defined inside a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original text is still missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be an artifact from another PR. Maybe the answer is to start a new PR with your changes for Effective Dart Design? Otherwise, keep working on getting this file (and patterns.md and patterns.md~) removed from the PR.
src/content/effective-dart/design.md
Outdated
@@ -519,10 +519,9 @@ class in a single library. | |||
|
|||
## Classes and mixins | |||
|
|||
Dart is a "pure" object-oriented language in that all objects are instances of | |||
classes. But Dart does not require all code to be defined inside a | |||
Classes. But Dart does not require all code to be defined inside a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "class" shouldn't be capitalized here.
Classes. But Dart does not require all code to be defined inside a | |
classes. But Dart does not require all code to be defined inside a |
src/content/effective-dart/design.md
Outdated
class—you can define top-level variables, constants, and functions like | ||
you can in a procedural or functional language. | ||
you can in a procedural or functional language |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing period:
you can in a procedural or functional language | |
you can in a procedural or functional language. |
src/content/effective-dart/design.md
Outdated
class, state that. Suffix the class name with `Base`, or mention it in the | ||
class's doc comment. | ||
Class modifiers like `final`, `base`, or `sealed` | ||
allow you to enforce whether a class can be extended directly in code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allow you to enforce whether a class can be extended directly in code. | |
enforce whether a class can be extended. |
src/content/effective-dart/design.md
Outdated
Rely on these modifiers rather than documentation to communicate and | ||
enforce your intent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rely on these modifiers rather than documentation to communicate and | |
enforce your intent. | |
Use these modifiers to communicate your intent, rather than relying on documentation. |
src/content/effective-dart/design.md
Outdated
|
||
|
||
### AVOID implementing a class that isn't intended to be an interface | ||
### ### DO use class modifiers to control if your class can be an interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your proposed rule is from the perspective of someone writing the library, but the original version was intended for those who are using a library. This section is talking about classs that aren't intended to be an interface, and while it would be nice if all such classes were using class modifiers to enforce these rules (as recommended above), it's still possible that a developer could run into these problems so I would vote to keep this section as-is.
src/content/language/patterns.md~
Outdated
@@ -0,0 +1,447 @@ | |||
--- | |||
title: Patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to make a copy of this file with a ~
at the end? Could you double check that your changes to this file are showing up correctly in the diff?
1)Made neccesary changes as said 2)Delete 'src/content/language/patterns.md~' file 3)Undid unnecessary indentation in 'src/content/language/patterns.md' file
4f8f2f7
to
41f58b3
Compare
@antfitch @johnpryan kindly take a look at the changes made |
2e73fae
to
5855642
Compare
@johnpryan does this rephrasing works ? |
1)Reverted rule3 to rule 1 2)let the 2nd section as it is
cc5ad65
to
13650d3
Compare
This is looking good, could you take a look and double check that there aren't extra changes committed that are unrelated? I see some changes to .json files that don't seem to belong here. |
yeah i removed the json file that was unnecessarily getting committed to this PR i dont know why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented on just one place that needs to say interface
rather than base
(base
doesn't restrict extends
, it restricts implements
).
src/content/effective-dart/design.md
Outdated
|
||
Class modifiers like `final`, `interface`, or `sealed` | ||
restrict how a class can be extended. | ||
For example, use `final class A {}` or `base class B {}` to prevent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be base class B {}
because that does not prevent extension outside the current library. It's interface
that prevents extends
(but allows implements
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed base class B {}
to interface class B{}
6340eff
to
45f381f
Compare
@eernstg just a follow-up comment that the said changes have been made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It looks good to me now.
I haven't pressed the 'Approve' button because I'm from the language team and the final acceptance should be provided by the site-www team.
4e99e3c
to
e586ada
Compare
@@ -199,8 +199,7 @@ the project: | |||
|
|||
* <a href='/effective-dart/design#avoid-defining-a-one-member-abstract-class-when-a-simple-function-will-do'>AVOID defining a one-member abstract class when a simple function will do.</a> | |||
* <a href='/effective-dart/design#avoid-defining-a-class-that-contains-only-static-members'>AVOID defining a class that contains only static members.</a> | |||
* <a href='/effective-dart/design#avoid-extending-a-class-that-isnt-intended-to-be-subclassed'>AVOID extending a class that isn't intended to be subclassed.</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shouldn't be removed AFAICT, can you make sure you re-ran the generator?
./dash_site effective-dart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep my bad i thought that this section was removed from the design.md
file that's why i remove the link too
lemme readd that
thanks for pointing it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
61a1b25
to
75e7570
Compare
…e to the previously failing task even when the anchor link is correct
Can you revert 4dc729d, thanks! |
…ctly, due to the previously failing task even when the anchor link is correct" This reverts commit 4dc729d.
b27935e
to
9f57c4a
Compare
@johnpryan done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
/gcbrun |
@MiniPiku it looks like a link is broken. Can you fix that? The link is in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last change to fix a broken link. Looks great!
c0cbf6f
to
eff55ab
Compare
@antfitch kindly take a look |
/gcbrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
This PR updates the Effective Dart Design Guidelines to align with Dart 3.0's introduction of class modifiers. The changes include:
For API Maintainers:
Removed the recommendation to document whether a class supports being extended or used as an interface.
Added guidance to use class modifiers (e.g., final, sealed, interface, mixin) to explicitly define a class's capabilities.
Emphasized that class modifiers should replace manual documentation for enforcing restrictions on extending or implementing classes.
For Users:
Removed guidelines advising users to avoid extending or implementing classes not meant for it, as these restrictions are now enforced by class modifiers in Dart 3.0.
This change ensures the guidelines are up-to-date with Dart's latest features and best practices.
Fixes: #6437