Skip to content

Implement generic function type syntax in dartdoc #1321

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

Closed
bwilkerson opened this issue Jan 10, 2017 · 25 comments
Closed

Implement generic function type syntax in dartdoc #1321

bwilkerson opened this issue Jan 10, 2017 · 25 comments
Assignees
Labels
customer-flutter Issues originating from important to Flutter P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@bwilkerson
Copy link
Member

This is the dartdoc-implementation issue for dart-lang/sdk#27527.

@devoncarew devoncarew added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Jan 10, 2017
@devoncarew
Copy link
Member

punted to 1.23

@mit-mit
Copy link
Member

mit-mit commented Mar 15, 2017

@devoncarew who should this be assigned to?

@jcollins-g
Copy link
Contributor

I'm willing to take a look, but it looks like from the description, it will be dependent on analyzer support.

@devoncarew
Copy link
Member

@jcollins-g, thanks! If this needs a new analyzer library, we can either backport the necessary work to the 0.29 release of analyzer, release an entirely new 0.30 version, or solve the issue by moving dartdoc into the sdk repo.

@devoncarew
Copy link
Member

It looks like dart_style (dart-lang/dart_style#563) will also need a version of the analyzer w/ generic function type support.

@mit-mit
Copy link
Member

mit-mit commented Mar 27, 2017

Analyzer is done!

@jcollins-g
Copy link
Contributor

I'll start looking at this today.

@dgrove
Copy link

dgrove commented Mar 28, 2017

The last full push to trunk is Wednesday early morning. Is this likely to land by then?

@jcollins-g
Copy link
Contributor

Implementation & tests complete, assuming I understand the feature correctly. Blocking on landing of cl 2778253002, landing a backport of this and related CLs to the analyzer 0.29 branch in SDK, publishing the new analyzer package, and then submitting my change to dartdoc. So it is going to be hard to get all that done in the next several hours.

Declared NewGenericTypedef as below:

/// A typedef with the new style generic function syntax.
typedef NewGenericTypedef<T> = List<T> Function<T>(int);

Screenshots:
screenshot from 2017-03-28 11 02 28
screenshot from 2017-03-28 11 03 24

@keertip
Copy link
Collaborator

keertip commented Mar 28, 2017

Looks good! See the generic type info 👍

@jcollins-g
Copy link
Contributor

@bwilkerson I think these are the blocking items we need from analyzer:

  • CL 2778253002 must land
  • Above CL and other CLs needed to make this analyze cleanly must be published in an analyzer package, so we can avoid errors like line 126-132 here

@jcollins-g
Copy link
Contributor

CL 2778253002 has landed.

With this change, old and new versions of dartdoc at least do not crash or have missing docs, though the older dartdoc does have a cosmetic problem with some extra whitespace in the parameter list which I fixed in the branch.

We can choose any of:

  • Do nothing from here on for 1.23, leaving the extra whitespace in dartdoc for the new syntax as a bug.
  • Switch the DEPS in the SDK to point to the branch for dartdoc
  • Wait for published versions of analyzer, then publish dartdoc, then update DEPS and ship the new dartdoc that way, either as a 1.23 cherrypick or with the next version.

@jcollins-g
Copy link
Contributor

Actually, it turns out the cosmetic problem was introduced after 0.9.12. So this should not be a blocking issue at all for 1.23 -- just use 0.9.12 dartdoc as it is currently configured.

We'll still need to get this untangled eventually for flutter and other non-SDK customers, so I'd prefer to leave the issue open until this actually can be committed to dartdoc head.

@devoncarew
Copy link
Member

Sounds like we still want to track, but can drop the 1.23 label.

@jcollins-g jcollins-g removed the 1.23 label Mar 28, 2017
@bwilkerson
Copy link
Member Author

The example given isn't a very good one because there are two type parameters with the same name. A better example would be

typedef NewGenericTypedef<T> = List<S> Function<S>(T);

The Dartdoc also appears to miss the fact that there are two lists of type parameters, so I think it needs some updating. Whether that needs to happen before 1.23 is someone else's call.

@jcollins-g
Copy link
Contributor

Here are some screenshots of a better example.

screenshot from 2017-03-28 14 32 59
screenshot from 2017-03-28 14 31 18

As for "two lists of type parameters", what would you expect to see here?

@jcollins-g
Copy link
Contributor

jcollins-g commented Mar 29, 2017

Discussed with Brian; since the type parameter is unresolved it makes sense to put that after NewGenericTypedef in the example above. (So it would be List<S> NewGenericTypedef<S>(T, int, bool) )

Which while a little confusing looking is also more accurate, as it answers the question "where did <S> come from". I'll put this in next.

@jcollins-g
Copy link
Contributor

jcollins-g commented Mar 29, 2017

Now that I understand the feature better I think this really does need to be improved.

New screenshots showing the change:

screenshot from 2017-03-29 09 35 42
screenshot from 2017-03-29 09 36 09

It's still not great; note how <S> appears in some places and <T> in others, which is confusing. But it seems more correct than before. I decided to place <S> in the library doc as that seems more consistent with how generic functions (without typedefs) are handled, but I'm open to changing that if there's something that's more intuitive people can suggest.

@jcollins-g
Copy link
Contributor

jcollins-g commented Mar 29, 2017

CL 2787513003 uploaded for analyzer, generic-function-types branch updated with new tests. dartdoc branch is still pending a published version of analyzer containing these changes.

@mit-mit
Copy link
Member

mit-mit commented Apr 3, 2017

@jcollins-g @bwilkerson where are we at with this one?

@jcollins-g
Copy link
Contributor

I'm going to tag the branch and manually test dartdoc merged with the SDK today, and we can ship an SDK that contains dartdoc (even though dartdoc head is broken).

@jcollins-g
Copy link
Contributor

jcollins-g commented Apr 3, 2017

proposed tag for 0.9.14-dev: b1f60db4e0d1550487d622dd62d8225c96682dc7, on branch generic-function-types

@devoncarew
Copy link
Member

@jcollins-g, with https://codereview.chromium.org/2795653003/, we're good to close this?

@jcollins-g
Copy link
Contributor

Now that the revision is merged to the SDK, I'm OK with removing the 1.23 tag. Don't want to close until we have an analyzer published package containing the changes.

@jcollins-g jcollins-g removed 1.23 status-blocked Blocked from making progress by another (referenced) issue labels Apr 5, 2017
@Hixie Hixie added the customer-flutter Issues originating from important to Flutter label May 5, 2017
@jcollins-g
Copy link
Contributor

Support is in published dartdoc with 0.12.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-flutter Issues originating from important to Flutter P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

7 participants