Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22850005:
DOM elements now have MDN comments if provided. (Closed)
|
Created:
7 years, 4 months ago by janicejl Modified:
7 years, 4 months ago CC:
Tate Mandel Visibility:
Public. |
DescriptionDOM elements now have MDN comments if provided.
BUG=
R=efortuna@google.com
Committed: https://code.google.com/p/dart/source/detail?r=26206
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 1
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:102: remove whitespace please. https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:377: if (domAnnotation.length != 1) return; are there places where we have more than one domAnnotation? That seems like an error on the library's part that should be addressed if so. https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:378: var domname = domAnnotation.single.parameters.single; domName https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:391: var mdnMemberList = mdnType['members'].where((e) => e['name'] == member); what about just calling firstWhere instead? https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:399: '<p class="mdn-note"><a href="' + mdnMember['url'].trim() + can this string and the one below be shared since everything is the same except the strings you're adding? https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:963: if (comment.isNotEmpty && comment != "") return; does isNotEmpty really not cover the case where coment == ""?
https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:102: On 2013/08/15 17:05:42, Emily Fortuna wrote: > remove whitespace please. Done. https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:377: if (domAnnotation.length != 1) return; On 2013/08/15 17:05:42, Emily Fortuna wrote: > are there places where we have more than one domAnnotation? That seems like an > error on the library's part that should be addressed if so. I do not think so. But it is possible for there to be no domAnnotation (not a dom element). https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:378: var domname = domAnnotation.single.parameters.single; On 2013/08/15 17:05:42, Emily Fortuna wrote: > domName Done. https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:391: var mdnMemberList = mdnType['members'].where((e) => e['name'] == member); On 2013/08/15 17:05:42, Emily Fortuna wrote: > what about just calling firstWhere instead? Done. https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:399: '<p class="mdn-note"><a href="' + mdnMember['url'].trim() + On 2013/08/15 17:05:42, Emily Fortuna wrote: > can this string and the one below be shared since everything is the same except > the strings you're adding? Done. https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:963: if (comment.isNotEmpty && comment != "") return; On 2013/08/15 17:05:42, Emily Fortuna wrote: > does isNotEmpty really not cover the case where coment == ""? Done.
https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:377: if (domAnnotation.length != 1) return; On 2013/08/15 17:45:19, janicejl wrote: > On 2013/08/15 17:05:42, Emily Fortuna wrote: > > are there places where we have more than one domAnnotation? That seems like an > > error on the library's part that should be addressed if so. > > I do not think so. But it is possible for there to be no domAnnotation (not a > dom element). Right, so firstWhere would be appropriate here, too, right? https://codereview.chromium.org/22850005/diff/8001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/8001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:373: if (item.comment != null && item.comment != '') return; can you use isNotEmpty here, too, instead?
https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/3001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:377: if (domAnnotation.length != 1) return; On 2013/08/15 18:13:05, Emily Fortuna wrote: > On 2013/08/15 17:45:19, janicejl wrote: > > On 2013/08/15 17:05:42, Emily Fortuna wrote: > > > are there places where we have more than one domAnnotation? That seems like > an > > > error on the library's part that should be addressed if so. > > > > I do not think so. But it is possible for there to be no domAnnotation (not a > > dom element). > > Right, so firstWhere would be appropriate here, too, right? Done. https://codereview.chromium.org/22850005/diff/8001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/8001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:373: if (item.comment != null && item.comment != '') return; On 2013/08/15 18:13:05, Emily Fortuna wrote: > can you use isNotEmpty here, too, instead? Done.
lgtm
Message was sent while issue was closed.
Committed patchset #4 manually as r26206 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/22850005/diff/12001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/22850005/diff/12001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:69: Map _mdn; Drive by review: Could one argue for a TODO: make this generic and perhaps pluggable? I could imagine other types of pluggable annotations (details of an auto-generated web service library). At a minimum, move MDN-specific code to its own library that's imported into the default impl?
Message was sent while issue was closed.
On 2013/08/15 18:59:57, kevmoo wrote: > https://codereview.chromium.org/22850005/diff/12001/pkg/docgen/lib/docgen.dart > File pkg/docgen/lib/docgen.dart (right): > > https://codereview.chromium.org/22850005/diff/12001/pkg/docgen/lib/docgen.dar... > pkg/docgen/lib/docgen.dart:69: Map _mdn; > Drive by review: > > Could one argue for a TODO: make this generic and perhaps pluggable? > > I could imagine other types of pluggable annotations (details of an > auto-generated web service library). > > At a minimum, move MDN-specific code to its own library that's imported into the > default impl? That's a nice suggestion, Kevin! We're hoping to improve our own specific documentation to the point that we'll be able to get rid of the external docs, such as MDN (since some of the names are different the linked in docs can sometimes be misleading), so in theory, ideally, this feature wouldn't be needed at all in the future, but we can certainly add a TODO to keep in mind as we see how things evolve. |
