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

Log trace on 400 and 500 codes #102

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Log trace on 400 and 500 codes #102

merged 1 commit into from
Aug 30, 2023

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Aug 30, 2023

This change is Reviewable

@Enkidu93 Enkidu93 requested a review from johnml1135 August 30, 2023 14:22
@johnml1135
Copy link
Collaborator

src/Serval.Shared/Controllers/HttpResultFilter.cs line 16 at r1 (raw file):

        public override Task OnResultExecutionAsync(ResultExecutingContext context, ResultExecutionDelegate next)
        {
            if (context.Result is ObjectResult r && r.StatusCode / 100 > 3)

PEMDAS makes my head spin - and is it int or float math? How about:
if ((context.Result is ObjectResult r) && (r.StatusCode >= 400))?

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

@Enkidu93
Copy link
Collaborator Author

src/Serval.Shared/Controllers/HttpResultFilter.cs line 16 at r1 (raw file):

        public override Task OnResultExecutionAsync(ResultExecutingContext context, ResultExecutionDelegate next)
        {
            if (context.Result is ObjectResult r && r.StatusCode / 100 > 3)

PEMDAS makes my head spin - and is it int or float math? How about: if ((context.Result is ObjectResult r) && (r.StatusCode >= 400))?

Haha, sure. I can do that. I think I even switched it to a ">= 400" but must have control-z-ed.

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

Fix as per PR --ECL
@johnml1135 johnml1135 force-pushed the http_response_logger branch from a2b8b95 to f203c74 Compare August 30, 2023 14:52
Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@johnml1135 johnml1135 merged commit e688077 into main Aug 30, 2023
1 check passed
@ddaspit ddaspit deleted the http_response_logger branch September 1, 2023 14:57
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.

2 participants