Use the logging package for dartdoc output #1518

kevmoo · 2017-10-17T00:07:25Z

No description provided.

kevmoo · 2017-10-17T03:32:24Z

Explicitly not using pkg/logging so I can send non-Strings to the listener

jcollins-g · 2017-10-17T14:33:32Z

https://github.com/dart-lang/logging/blob/master/lib/logging.dart#L158

Why is this not sufficient?

kevmoo · 2017-10-17T14:38:57Z

Because https://github.com/dart-lang/logging/blob/50b32dc828d9ad4291d859c8cc6a80ed445cf5b2/lib/logging.dart#L173

…

On Tue, Oct 17, 2017 at 7:38 AM, jcollins-g ***@***.***> wrote: https://github.com/dart-lang/logging/blob/master/lib/logging.dart#L158 Why is this not sufficient? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1518 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCioOySiFu3H_PiZGJNyekoYU6D92Vks5stLvIgaJpZM4P7Z5S> .

jcollins-g · 2017-10-17T15:35:07Z

I'll need more of an explanation than that. Why do you need to be able to log non-string-convertible objects?

kevmoo · 2017-10-17T18:48:04Z

The idea: push the "text or JSON" decision to the listener. When a component logs it either sends a String and/or something JSON-able.

kevmoo · 2017-10-17T23:02:36Z

@jcollins-g – thoughts?

jcollins-g

I think I see where you're going, but the implementation is problematic as you're basically recreating a lot of the logging module inside Dartdoc. I would prefer if you extended the logging package's class through inheritance and override its 'log' method with one more suited to your needs, or alter the logging package if you can do that in a reasonably compatible manner. Both of these options are better for our ecosystem and since this isn't an urgent issue I'd prefer that.

Another alternative is to make use of the logging module's existing support for passing closures and when you want to pass something in, wrap it in a closure that knows what to do based on dartdoc's command line flags -- convert to JSON or use ordinary string logging. This could be done with a lightweight wrapper around the logging module. I'm guessing this would be more complex than the other options when implemented though.

I'm not familiar enough with the logging module to know for sure which of those alternatives look best when implemented, but I'm pretty sure one of them will work out better in the long run.

jcollins-g · 2017-10-17T14:32:07Z

bin/dartdoc.dart


-  var readme = args['sdk-readme'];
+  var readme = args['sdk-readme'] as String;


prefer: String readme = args['sdk-readme']; (or leave original)

jcollins-g · 2017-10-18T14:19:57Z

pubspec.yaml

@@ -1,6 +1,6 @@
 name: dartdoc
 # Also update the `version` field in lib/dartdoc.dart.
-version: 0.14.1
+version: 0.14.2-dev


add entry to changelog if you like but I prefer to avoid updating the version number for non-breaking changes until release.

I still think this is poor CM practice but if it is standard for Dart package development, we can do this. A better way might be to put this in a separate PR.

I'd like to see future -dev changes made in separate PRs to make reverts easier.

But if we're going to keep this one, the version number in lib/dartdoc.dart should also change.

Happy to do a rebase cleanup...if you have no other comments

kevmoo · 2017-10-18T18:20:35Z

I'd have to write more code to extends pkg/logging classes (both Logger and LogRecord). We'd also end up with a leaky abstraction, since I'd also have to introduce my own stream of events that's separate from Logger. Or I'd have to implement Logger – and then implement all of the members...

The todo here is to make pkg/logging more extensible. In the mean time, I think this is the cleanest solution.

kevmoo · 2017-10-19T03:56:49Z

@jcollins-g – further thoughts?

jcollins-g · 2017-10-20T14:24:50Z

I still don't think this is urgent enough to develop an entirely new logging module here even as a stopgap -- and I don't see it taking "more code" to extend the logging module to enable JSON logging. Whether you have a json object or not, the warnings will get transformed to a string and written out, which is what the existing logging module does. If json objects do not have a "toString()" that does that it's easy enough to add as an extension to logger.

I see no reason to introduce a second stream of events or to reimplement Logger in its entirety.

kevmoo · 2017-10-23T21:58:35Z

@jcollins-g – I understand logging a bit better now.

Trying again – with the logging package. PTAL

jcollins-g

This is much better, thanks! Only one question of note.

jcollins-g · 2017-10-24T15:29:36Z

pubspec.yaml

@@ -1,6 +1,6 @@
 name: dartdoc
 # Also update the `version` field in lib/dartdoc.dart.
-version: 0.14.1
+version: 0.14.2-dev


I still think this is poor CM practice but if it is standard for Dart package development, we can do this. A better way might be to put this in a separate PR.

jcollins-g · 2017-10-24T15:32:36Z

test/compare_output_test.dart

@@ -145,7 +145,7 @@ void main() {
      final sep = '.'; // We don't care what the path separator character is
      final firstUnfoundExample = new RegExp('warning: lib${sep}example.dart: '
          '@example file not found.*test_package${sep}dog${sep}food.md');
-      if (!result.stderr.contains(firstUnfoundExample)) {
+      if (!result.stdout.contains(firstUnfoundExample)) {


Do we know why this is now stdout? Warning prints should go to stderr.

Disagree! if dartdoc has a error/crash that should go to stderr

See https://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout

stderr should be for diagnostics.

If I pipe the output from dartdoc to a file, I want the hints and the warnings all together.

As discussed offline, this interpretation doesn't follow from the intended purpose of dartdoc, which is not to generate warnings, but documentation. Given that, the warnings are diagnostic information, and should remain on stderr.

See d8e2a66#r146899745

jcollins-g · 2017-10-24T15:34:33Z

bin/dartdoc.dart

+
+  logging.Logger.root.onRecord.listen((record) {
+    if (record.level == progressLevel) {
+      if (showProgress && stopwatch.elapsed.inMilliseconds > 250) {


This definitely makes the progress ticker less annoying.

kevmoo · 2017-10-24T16:44:57Z

I still think this is poor CM practice but if it is standard for Dart package development, we can do this. A better way might be to put this in a separate PR.

It helps one understand if the package is in a "work in progress" state. -dev means between releases. I think it's super helpful.

kevmoo · 2017-10-24T21:18:58Z

@jcollins-g PTAL

kevmoo · 2017-10-24T22:00:37Z

Fixed travis oops – I think

kevmoo · 2017-10-25T15:42:44Z

test/compare_output_test.dart

@@ -145,7 +145,7 @@ void main() {
      final sep = '.'; // We don't care what the path separator character is
      final firstUnfoundExample = new RegExp('warning: lib${sep}example.dart: '
          '@example file not found.*test_package${sep}dog${sep}food.md');
-      if (!result.stdout.contains(firstUnfoundExample)) {
+      if (!result.stderr.contains(firstUnfoundExample)) {


@jcollins-g – see here – that change got reverted...

kevmoo · 2017-10-31T19:42:18Z

Look okay, @jcollins-g ?

kevmoo requested a review from jcollins-g October 17, 2017 00:07

googlebot added the cla: yes Google CLA check succeeded. label Oct 17, 2017

kevmoo force-pushed the logging branch 2 times, most recently from f5a92cb to e921101 Compare October 17, 2017 03:31

jcollins-g suggested changes Oct 18, 2017

View reviewed changes

kevmoo force-pushed the logging branch from e921101 to 26f17b4 Compare October 18, 2017 18:27

jcollins-g closed this Oct 20, 2017

kevmoo reopened this Oct 23, 2017

jcollins-g suggested changes Oct 24, 2017

View reviewed changes

kevmoo commented Oct 25, 2017

View reviewed changes

jcollins-g changed the title ~~Logging~~ Use the logging package for dartdoc output Oct 27, 2017

kevmoo force-pushed the logging branch 2 times, most recently from 19c7813 to 4404f47 Compare October 27, 2017 17:52

kevmoo added 3 commits October 27, 2017 14:11

Bump version to 1.14.2-dev

ebdf2bc

Rename _printHelp -> _printHelpAndExit to be clear about behavior

969a996

Send most build-time output through logging

c79c23e

kevmoo force-pushed the logging branch from 4404f47 to c79c23e Compare October 27, 2017 21:11

jcollins-g approved these changes Oct 31, 2017

View reviewed changes

kevmoo merged commit c79c23e into master Oct 31, 2017

kevmoo deleted the logging branch October 31, 2017 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the logging package for dartdoc output #1518

Use the logging package for dartdoc output #1518

kevmoo commented Oct 17, 2017

kevmoo commented Oct 17, 2017

jcollins-g commented Oct 17, 2017

kevmoo commented Oct 17, 2017 via email

jcollins-g commented Oct 17, 2017

kevmoo commented Oct 17, 2017

kevmoo commented Oct 17, 2017

jcollins-g left a comment

jcollins-g Oct 17, 2017

kevmoo Oct 18, 2017

jcollins-g Oct 18, 2017

jcollins-g Oct 24, 2017

jcollins-g Oct 27, 2017

kevmoo Oct 27, 2017

kevmoo commented Oct 18, 2017

kevmoo commented Oct 19, 2017

jcollins-g commented Oct 20, 2017

kevmoo commented Oct 23, 2017

jcollins-g left a comment

jcollins-g Oct 24, 2017

jcollins-g Oct 24, 2017

kevmoo Oct 24, 2017

jcollins-g Oct 25, 2017

kevmoo Oct 25, 2017

jcollins-g Oct 24, 2017

kevmoo commented Oct 24, 2017

kevmoo commented Oct 24, 2017

kevmoo commented Oct 24, 2017

kevmoo Oct 25, 2017

kevmoo commented Oct 31, 2017


		var readme = args['sdk-readme'];
		var readme = args['sdk-readme'] as String;

Use the logging package for dartdoc output #1518

Use the logging package for dartdoc output #1518

Conversation

kevmoo commented Oct 17, 2017

kevmoo commented Oct 17, 2017

jcollins-g commented Oct 17, 2017

kevmoo commented Oct 17, 2017 via email

jcollins-g commented Oct 17, 2017

kevmoo commented Oct 17, 2017

kevmoo commented Oct 17, 2017

jcollins-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevmoo commented Oct 18, 2017

kevmoo commented Oct 19, 2017

jcollins-g commented Oct 20, 2017

kevmoo commented Oct 23, 2017

jcollins-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevmoo commented Oct 24, 2017

kevmoo commented Oct 24, 2017

kevmoo commented Oct 24, 2017

Choose a reason for hiding this comment

kevmoo commented Oct 31, 2017