Skip to content

FlutterLogoDecoration.isComplex isn't showing its documentation #1394

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
Hixie opened this issue May 3, 2017 · 10 comments · Fixed by #1466
Closed

FlutterLogoDecoration.isComplex isn't showing its documentation #1394

Hixie opened this issue May 3, 2017 · 10 comments · Fixed by #1466
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

@Hixie
Copy link
Contributor

Hixie commented May 3, 2017

https://docs.flutter.io/flutter/painting/FlutterLogoDecoration-class.html

hashCode and isComplex have no docs, but they are inherited from a class that does.

Weirdly toString, which is in the same situation, does have docs.

@jcollins-g
Copy link
Contributor

This is probably because hashCode and isComplex are properties, and toString is a method. I'll look into this.

@jcollins-g
Copy link
Contributor

jcollins-g commented May 4, 2017

This is part of the constellation of issues relating to properties, including documentation on getters/setters, and inheritance, with symptoms like #1116, #1352, #1337, #1103, and probably more. Dartdoc does not handle the synthetic nature of fields very well, especially when there is inheritance and overrides and that section needs some focused attention, which hopefully once the major canonicalization issues are dealt with I can address.

In particular for this bug, getterSetterCombo's getterSetterDocumentationComment knows nothing about inheritance and it needs to.

@Hixie Hixie added 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 labels May 5, 2017
@jcollins-g
Copy link
Contributor

jcollins-g commented Jun 8, 2017

Working on this actively now. General strategy is to split Accessor out into InheritableAccessor (with the Inheritable mixin, for direct use by Class+Field) and Accessor, change the factories to generate the right classes at the right time, and rewire the documentation comment fetching to use the new inheritance+canonicalization goodness introduced in previous work.

@jcollins-g jcollins-g self-assigned this Jun 8, 2017
@jcollins-g
Copy link
Contributor

jcollins-g commented Jun 14, 2017

There is still a lot of work to do as the implications of this change ripple through dartdoc's code, but things starting to work well enough to generate screenshots:

From this bug:
flutterlogodecoration

A case of complex behavior for a synthetic property where the getter and setter have different documentation comments and the getter is not overridden but the setter is (AnimationController.value)
animationcontroller

Some questions are coming up for @Hixie and other interested folk:

  • If a property has a getter or setter inherited (but not both), how should it be marked? There doesn't seem to be a "half-italic" font-style setting in css. :-)
  • How should one-line docs be handled for complex cases? I coded up something that doesn't seem to look horrible, above, but am open to suggestions.
  • How should annotations be displayed? Currently we just display the union of both sets of annotations for the field, but maybe they should be divided somehow?

@jcollins-g
Copy link
Contributor

Some discussions with @devoncarew brought up the idea of splitting up properties to some degree in the docs, either explicitly all the time or for synthetic properties (those including explicitly defined getters or setters). I'm not a super huge fan of that because Dart doesn't treat the getter/setter combination separately when you use it and I worry that might be confusing, but it might be an acceptable tradeoff if it's believed splitting that up will actually be less confusing than the other problems listed above.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 14, 2017

If a property has a getter or setter inherited (but not both), how should it be marked?

By "not inherited" do you mean "overridden" or "introduced"? Like, are you talking about a case where a subclass overrides the getter but not the setter, or a case where in the superclass the property is read-only but the subclass introduces a setter?

In either case, I would handle it by styling it according to the member that changed status. So if it went from getter-only to inherited-getter+new-setter, I'd style it the same way as a new method. If it went from getter-and-setter to inherited-getter+override-setter, I'd style it the same way as an overridden method.

How should one-line docs be handled for complex cases? I coded up something that doesn't seem to look horrible, above, but am open to suggestions.

For getter/setter pairs, I'd only include the getter's docs in the overview page. It's rare for setters to have docs and when they do they tend to be just additional information that depends on knowing the getter docs anyway.

How should annotations be displayed? Currently we just display the union of both sets of annotations for the field, but maybe they should be divided somehow?

Union seems fine to me for the overview page. Split them up on the details page.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 14, 2017

If the suggestions above are a pain to implement, let me know, I don't feel super-strongly about this and could easily come up with alternative conclusions given some guidance as to what specific solutions are best avoided.

@jcollins-g
Copy link
Contributor

jcollins-g commented Jun 14, 2017 via email

@Hixie
Copy link
Contributor Author

Hixie commented Jun 14, 2017

a case where the setter had some interesting side-effects mentioned in the one-liner that to my eyes seemed unfortunate to skip on

I agree that that is an interesting case, but the reality of it is that lots of the docs have interesting details that are unfortunate to skip on buried in paragraph 3 or some subsection in the docs or whatnot. We can only do so much in one sentence. Most setters have no docs or have nitty gritty docs which it would seem weird to surface in the summary, IMHO. The getters have one-liners that are written with the assumption they'll be shown for both (e.g. if it's read-write they won't generally start "returns...", they'll just say what the property is about).

The new/override/inherited style difference is primarily about telling people "this property is probably not interesting, it's the same as the superclass" vs "this is relevant to your interests in this class", with a subcategorisation of "this is interesting because it's new here" and "this is interesting because it's a specialisation of the superclass". Adding a getter or setting to a property that already existed seems to me to be more of the "this is interesting because it's a specialisation" kind than the "this is entirely new" kind. You can think of adding a setter as overriding the property's behaviour, basically. People view the property as one thing, not as the setter and getter being unrelated features, and that's presumably why dartdocs document properties as one thing instead of as separate getters and setters.

So I would say:

  • if getter and setter are both inherited: inherited style
  • if getter and setter are both new: new style
  • otherwise, either getter is overriden, or setter is overriden, or getter is new and setter is inherited, or setter is new and getter is inherited: override style.

@jcollins-g
Copy link
Contributor

SGTM, will let you know once I have something worth looking at in more detail.

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

Successfully merging a pull request may close this issue.

2 participants