Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

remove [new from docs #8986

Merged
merged 8 commits into from
May 17, 2019
Merged

remove [new from docs #8986

merged 8 commits into from
May 17, 2019

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented May 16, 2019

Missed these in the PR where I got rid of optional news.

@dnfield dnfield requested review from Hixie and a14n May 16, 2019 16:40
@a14n
Copy link
Contributor

a14n commented May 16, 2019

I'm not such how dartdoc handles comment references (as dartdoc and navigation don't always have the same behaviour - see dart-lang/sdk#36974) but in code navigation [new Aaa] brings to Aaa constructor and [Aaa] brings you to the class. So I'm not sure the result is what you want.

@dnfield
Copy link
Contributor Author

dnfield commented May 16, 2019

For named constructors you definitely don't need the new - I've seen that working. For default constructors I'm not sure what's better. I could take this out for defaults.

/cc @gspencergoog who might know more on this

@gspencergoog
Copy link
Contributor

I think this is fine. In IntelliJ at least if you Ctrl-click on [new Size], it takes you to the Size class declaration anyhow, not the default constructor. And named constructors work properly too by taking you to the named constructor and not the class declaration. Dartdoc has different behavior: a link to [new Size] takes you to the default constructor page, and [Size] takes you to the class page.

We actually use that difference in a few places (e.g. see TextFormField where it says "For documentation about the various parameters, see the TextField class and new TextField, the constructor.")

I'm not sure the distinction is all that relevant most times, but when it is, it's important to go where the user thinks they're going. I'd just make sure to look at each instance of removing the new and see if it makes sense for that case or not.

@dnfield
Copy link
Contributor Author

dnfield commented May 16, 2019

Updated to prevserve new where it seems to make sense.

@@ -1689,7 +1689,7 @@ class RSTransform {
/// to factor out the computations of the sine and cosine of the rotation
/// (which are computed each time this constructor is called) and reuse them
/// over multiple [RSTransform] objects, it may be more efficient to directly
/// use the more direct [new RSTransform] constructor instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it refers to constructor here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@a14n a14n left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments

@@ -3553,7 +3553,7 @@ class Canvas extends NativeFieldWrapperClass2 {
///
/// To align the text, set the `textAlign` on the [ParagraphStyle] object
/// passed to the [new ParagraphBuilder] constructor. For more details see
/// [TextAlign] and the discussion at [new ParagraphStyle].
Copy link
Contributor

Choose a reason for hiding this comment

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

The discussion mentioned seems to be on new ParagraphStyle

lib/ui/text.dart Outdated
/// The width influences how ellipses are applied. See the discussion at [new
/// ParagraphStyle] for more details.
/// The width influences how ellipses are applied. See the discussion at
/// [ParagraphStyle] for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

/// See the discussion of the `maxLines` and `ellipsis` arguments at [new
/// ParagraphStyle].
/// See the discussion of the `maxLines` and `ellipsis` arguments at
/// [new ParagraphStyle].
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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 one is ok, right? I just moved new down to the next line.

Copy link
Contributor

Choose a reason for hiding this comment

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

right. sorry

@dnfield dnfield merged commit 065fe5b into flutter:master May 17, 2019
@dnfield dnfield deleted the more_new branch May 17, 2019 02:14
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 17, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request May 17, 2019
…2868)

flutter/engine@b6981a3...065fe5b

git log b6981a3..065fe5b --no-merges --oneline
065fe5b remove unnecessary `[new` from docs (flutter/engine#8986)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
@jamesderlin
Copy link
Contributor

BTW, also see dart-lang/dartdoc#1850.

@Hixie
Copy link
Contributor

Hixie commented May 17, 2019

Dartdoc has different behavior: a link to [new Size] takes you to the default constructor page, and [Size] takes you to the class page.

Dartdoc is canonical.

Whatever we do, please make sure it works in dartdocs (i.e. that we can link to constructors when we need to), and that it is documented in the style guide.

huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019
kiku-jw pushed a commit to kiku-jw/flutter that referenced this pull request Jun 14, 2019
…utter#32868)

flutter/engine@b6981a3...065fe5b

git log b6981a3..065fe5b --no-merges --oneline
065fe5b remove unnecessary `[new` from docs (flutter/engine#8986)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants