Skip to content

Code Quality: Made private UserSettingsService property into a readonly field #11009

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

Closed

Conversation

QuaintMako
Copy link
Contributor

Resolved / Related Issues

What has been done

  • Replaced private IUserSettingsService UserSettingsService { get; } = Ioc.Default.GetRequiredService<IUserSettingsService>(); by private readonly IUserSettingsService userSettingsService = Ioc.Default.GetRequiredService<IUserSettingsService>();
  • Inverted one if

Validation
How did you test these changes?

  • Built and ran the app.

hecksmosis
hecksmosis previously approved these changes Jan 14, 2023
Copy link
Contributor

@hecksmosis hecksmosis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. I think you should remove braces of single-line ifs
Edit. I think that private non-static fields should be prefixed by '_'. See 2. Naming notation for variables of code style guidelines

@QuaintMako
Copy link
Contributor Author

Edit. I think that private non-static fields should be prefixed by '_'. See 2. Naming notation for variables of code style guidelines

From what I see, this rule is not enforced anymore, most of the code base doesn't use it. If we are to do it, that's gonna be a huge cost.

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

LGTM

@hecksmosis
Copy link
Contributor

@QuaintMako Can you fix the compilation error?

@hecksmosis
Copy link
Contributor

Line 135 of FoldersViewModel.cs

hecksmosis
hecksmosis previously approved these changes Jan 21, 2023
Copy link
Contributor

@hecksmosis hecksmosis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 changed the title Code Quality : Made private UserSettingsService property into a readonly field. Code Quality: Made private UserSettingsService property into a readonly field. Jan 29, 2023
@QuaintMako QuaintMako dismissed stale reviews from ferrariofilippo and hecksmosis via a791d3f January 30, 2023 15:56
Copy link
Contributor

@hecksmosis hecksmosis left a comment

Choose a reason for hiding this comment

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

LGTM, but can you remove the FilesLauncher.exe, I don't think it's needed.

@yaira2
Copy link
Member

yaira2 commented Feb 2, 2023

@QuaintMako can you resolve the merge conflicts?

@yaira2 yaira2 changed the title Code Quality: Made private UserSettingsService property into a readonly field. Code Quality: Made private UserSettingsService property into a readonly field Feb 2, 2023
@QuaintMako QuaintMako closed this Feb 3, 2023
@QuaintMako QuaintMako deleted the SettingServicesReadonly branch February 21, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants