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

Cleanup of error response generation #1030

Merged
merged 13 commits into from
Dec 4, 2023

Conversation

veloman-yunkan
Copy link
Collaborator

This PR contains preparatory cleanup work before implementing translation of error pages (#1019).

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: Patch coverage is 29.88506% with 61 lines in your changes missing coverage. Please review.

Project coverage is 38.95%. Comparing base (f8aae39) to head (5f27b4b).
Report is 186 commits behind head on main.

Files with missing lines Patch % Lines
src/server/internalServer.cpp 0.00% 6 Missing and 34 partials ⚠️
src/server/response.cpp 63.63% 0 Missing and 12 partials ⚠️
src/server/internalServer_catalog.cpp 0.00% 2 Missing and 6 partials ⚠️
src/server/i18n.h 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   38.88%   38.95%   +0.07%     
==========================================
  Files          58       58              
  Lines        3991     3958      -33     
  Branches     2204     2181      -23     
==========================================
- Hits         1552     1542      -10     
+ Misses       1090     1086       -4     
+ Partials     1349     1330      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Just two small changes but LM(*)GTM (*mostly)

@veloman-yunkan veloman-yunkan force-pushed the cleanup_of_error_response_generation branch from b4fa17d to 9c174e4 Compare November 29, 2023 17:21
It makes little sense to pass the verbosity control to the `Response`
constructor if it is used only in `Response::send()`.
It turned out that ContentResponse::m_root is no longer used.

At this point, the root parameter is dropped only from the 3-ary variant
of ContentResponse::build(), so that its all call sites are
automatically discovered by the compiler (and updated manually).
Including the other (4-ary) variant of ContentResponse::build() in this
change might result in the semantic change of expressions like
`ContentResponse::build(x, y, z)` and failure to update them.
@veloman-yunkan veloman-yunkan force-pushed the cleanup_of_error_response_generation branch from 9c174e4 to 5f27b4b Compare November 29, 2023 17:32
@kelson42
Copy link
Collaborator

kelson42 commented Dec 3, 2023

@mgautierfr Good to merge?

@mgautierfr mgautierfr merged commit 0d2b6b3 into main Dec 4, 2023
@mgautierfr mgautierfr deleted the cleanup_of_error_response_generation branch December 4, 2023 09:59
@kelson42 kelson42 added this to the 13.1.0 milestone Jan 29, 2024
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