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

Indent log messages according to the request status #382

Merged
merged 11 commits into from
Jul 1, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Jun 26, 2019

Tracking PR for #379

@XVincentX XVincentX requested a review from marbemac June 26, 2019 10:09
@XVincentX XVincentX changed the title ## Checklist Indent log messages according to the request status Jun 26, 2019
@XVincentX XVincentX force-pushed the feat/logging-indent branch from 2bede46 to b9229ec Compare June 26, 2019 10:48
@XVincentX XVincentX force-pushed the feat/logging-indent branch from b1a6b58 to c898bb9 Compare June 26, 2019 13:37
philsturgeon
philsturgeon previously approved these changes Jun 26, 2019
@XVincentX
Copy link
Contributor Author

Let's wait for the big boss

@XVincentX XVincentX force-pushed the feat/logging-indent branch from d8de79c to 5ab7bbc Compare June 27, 2019 09:19
@XVincentX XVincentX requested a review from philsturgeon June 27, 2019 13:12
philsturgeon
philsturgeon previously approved these changes Jun 27, 2019
@marbemac
Copy link
Contributor

60253716-e983f380-98cc-11e9-86e8-a65afa4511aa

Looks great! ONE last comment, I promise :).

In the last screenshot above, it looks like the request resulted in an error, is that correct? I suggest that everything related to a request be nested underneath it, including the final error there. As it stands, it's not clear that that error is the final result of the request above it. Does that make sense?

@XVincentX
Copy link
Contributor Author

@marbemac That isn't really that easy because the error is detected from the HTTP Server which has the informations to display all the data — but it's on a different level. Indenting only that part might be difficult. Let me try that though, I'll get back to you.

@philsturgeon
Copy link
Contributor

@XVincentX I think we should probably kick this over the line and improve it later if anyone points it out? @marbemac hopefully you're ok with that? I agree this would be nicer but the PR is a big improvement in its own right. We gotta get moving on the e2e test suite.

@XVincentX
Copy link
Contributor Author

Yeah, I'm ok merging this — the reason why I did not it's because it's not blocking anybody, so it can stay here. I'd love to see this merged though!

@XVincentX
Copy link
Contributor Author

image

@philsturgeon @marbemac

I've implemented the change you requested with an additional property. I have a better idea to implement this in the future but I guess for now let's keep it in this way.

@XVincentX XVincentX requested a review from philsturgeon July 1, 2019 08:36
Copy link
Contributor

@philsturgeon philsturgeon left a comment

Choose a reason for hiding this comment

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

This looks fantastic, amazing job!

@XVincentX XVincentX merged commit fa92140 into master Jul 1, 2019
@XVincentX XVincentX deleted the feat/logging-indent branch July 1, 2019 08:57
@marbemac
Copy link
Contributor

marbemac commented Jul 2, 2019

Sorry to hold it up, just seeing this - looks fantastic! Great job @XVincentX 👏 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants