Skip to content
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

Add responseSentTime to response format #95

Merged
merged 4 commits into from Jan 10, 2018

Conversation

@berzniz
Copy link
Contributor

berzniz commented Oct 10, 2017

Added responseSentTime to the response format and updated the unit tests. Questions left open:

  1. This could be a breaking change for people who parse the logs. Should this be opt-in?
  2. Should we tell which time is which in the log format?

Ideally, it should not be a breaking change and we will be able to pass in a format string for responses. Is this commit a good start?

Addresses #94

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 11, 2017

  1. I feel like if you are parsing strings... you'd have your own logger at this point. So I'd say just leave it as is.
  2. Yes, put some really short copy in front of each value so you can tell which is which.

I'd just make this a semver major to avoid any potential breakages.

@berzniz

This comment has been minimized.

Copy link
Contributor Author

berzniz commented Oct 11, 2017

  1. 👍
  2. I'll add the short copy, but what do you think should it be?
  3. Tests are not passing which is weird, the error is The following leaks were detected:WebAssembly, any idea?
@arb arb self-assigned this Oct 12, 2017
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 12, 2017

  1. It's your PR, figured you'd have an idea what the short copy should be
  2. While you're in there, try updating some of the development dependencies.
berzniz added 2 commits Oct 15, 2017
@berzniz

This comment has been minimized.

Copy link
Contributor Author

berzniz commented Oct 15, 2017

I added the label in front of the response times and updated the dev dependencies (+fixed one linting error)

Travis still fails, I think we need to run it without cache so new dependencies get installed. I can't configure it to clear the cache

@berzniz

This comment has been minimized.

Copy link
Contributor Author

berzniz commented Nov 8, 2017

Any updates on this? Are there any changes required?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 8, 2017

Travis is still failing. I'm not sure what to do about it but this PR won't land until we get a clean run.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 5, 2017

If you have any interest in landing this, please fix merge conflicts and get CI passing, if not, please close the PR.

@berzniz

This comment has been minimized.

Copy link
Contributor Author

berzniz commented Dec 5, 2017

Fixed the merge conflicts and the CI is passing

@arb arb added this to the 7.0.0 milestone Jan 10, 2018
@arb arb merged commit eb54cd1 into hapijs:master Jan 10, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@berzniz berzniz deleted the berzniz:add_responseSentTime branch Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.