Skip to content

Add JSON logging #1531

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

Merged
merged 3 commits into from
Nov 3, 2017
Merged

Add JSON logging #1531

merged 3 commits into from
Nov 3, 2017

Conversation

kevmoo
Copy link
Member

@kevmoo kevmoo commented Nov 1, 2017

No description provided.

@kevmoo kevmoo requested a review from jcollins-g November 1, 2017 00:02
@googlebot googlebot added the cla: yes Google CLA check succeeded. label Nov 1, 2017
@kevmoo
Copy link
Member Author

kevmoo commented Nov 1, 2017

@jcollins-g – I was pondering that this case might be nice to export everything to stdout – just so it's easy to pipe to a file or another process.

...let's discuss

CHANGELOG.md Outdated
@@ -1,3 +1,6 @@
## 0.14.2
Copy link
Contributor

Choose a reason for hiding this comment

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

remove version number from CL; you don't know what version this will be published as.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is the pattern we've used through packages.

First change post-release: bump version & -dev

So 1.2.3 becomes 1.2.4-dev

The changelog entries follow:

## 1.2.4

Then if it becomes a breaking release...

1.2.4-dev become 2.0.0-dev

and update the changelog accordingly

That's how we (try) to manage all packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, didn't know that was a standard practice as well. we'll try and do it that way then.

Copy link
Member

Choose a reason for hiding this comment

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

Not to muddy the waters, but I use ## unreleased as a placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

Request adding an end-to-end test of this feature that validates good JSON is written when generating docs for the test package.

}
});

print(JSON.encode(output));
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing this to stdout is OK if you can redirect stdout internally in dartdoc to something else when writing json, so an errant print statement somewhere doesn't break your JSON.

Indeed, that's how this should be implemented generally, assuming that's possible in dart. That way you don't have to require that all output goes through this library.

@kevmoo
Copy link
Member Author

kevmoo commented Nov 3, 2017

Request adding an end-to-end test of this feature that validates good JSON is written when generating docs for the test package.

On it!

@kevmoo
Copy link
Member Author

kevmoo commented Nov 3, 2017

@jcollins-g – PTAL

@jcollins-g
Copy link
Contributor

This looks pretty good now. Use zones to redirect errant print statements as discussed and then this will be fine.

@kevmoo
Copy link
Member Author

kevmoo commented Nov 3, 2017

PTAL

@kevmoo kevmoo merged commit f2fddbb into master Nov 3, 2017
@kevmoo kevmoo deleted the json_logging branch November 3, 2017 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants