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

Polish codes in UI project. #143

Merged
merged 4 commits into from
Sep 13, 2024
Merged

Polish codes in UI project. #143

merged 4 commits into from
Sep 13, 2024

Conversation

mo-esmp
Copy link
Member

@mo-esmp mo-esmp commented Sep 7, 2024

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.

@mo-esmp mo-esmp requested a review from followynne September 7, 2024 12:16
@followynne
Copy link
Member

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?
  • I see some tests are failing, probably there's some typo that should be fixed 👍

@mo-esmp
Copy link
Member Author

mo-esmp commented Sep 9, 2024

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.
image

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.
image

@followynne
Copy link
Member

@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!

@mo-esmp mo-esmp merged commit 8fe464c into master Sep 13, 2024
4 checks passed
@mo-esmp mo-esmp deleted the feature/refactoring branch September 13, 2024 07:36
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