Closed mo-esmp closed 2 months ago
Hi @mo-esmp
it looks ok to me, I have just a couple of questions before approving 😉
the use of the direct type instead of var keyword on some variables , is it a style choice?
Nowadays, I prefer using direct types instead of var
. It seems like a micro-optimization to me because it saves a bit of time figuring out what the type would be. 😄
Regarding the broken tests, I have moved the HttpContext
null check from the method level to the class constructor level.
I believe these tests are not reflective of realistic scenarios because the HttpContext
does not change during request processing. It would be more appropriate to configure the IHttpContextAccessor
at the method level rather than in the test class constructor (to have the isolation). Please let me what do you think on this.
@mo-esmp
Thanks for the explanation on the var!
About the test, I agree with your point, in the specific broken test it was just done for a matter of keeping together the same scoped test 😄 I did a small refactoring to integrate the new variable management and now it works fine!
Before refactoring the SQL query generation for SQL-based providers, I chose to refine the code and standardize the coding style to prevent a large pull request for the subsequent one.