Skip to content

Codebase: Make private service fields readonly #11011

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

Merged
merged 8 commits into from
Feb 2, 2023

Conversation

hecksmosis
Copy link
Contributor

Resolved / Related Issues
Items resolved / related issues by this PR.

Validation
How did you test these changes?

  • Built and ran the app

@hecksmosis hecksmosis changed the title Set readonly services Codebase: Make private service fields readonly Jan 14, 2023
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.

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

QuaintMako
QuaintMako previously approved these changes Jan 20, 2023
@QuaintMako
Copy link
Contributor

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

This I am not sure. This notation has not been enfored for a while on the project (at least for the few months I have been active here). If we decide to enforce it, it will ask for a real effort to refactor the whole project in this direction. A decision should be taken regarding this in a proper discussion.

@yaira2
Copy link
Member

yaira2 commented Feb 1, 2023

Can you resolve the merge conflicts?

@hecksmosis
Copy link
Contributor Author

Done

yaira2
yaira2 previously approved these changes Feb 2, 2023
Copy link
Member

@yaira2 yaira2 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 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Feb 2, 2023
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.

There are also other services to be changed:

  • IFileTagsSettingsService of FileTagsService
  • IFileTagsSettingsService of AboutViewModel

@yaira2 yaira2 added needs - code review and removed ready to merge Pull requests that are approved and ready to merge labels Feb 2, 2023
@hecksmosis
Copy link
Contributor Author

That should be everything (according to the quick Visual Studio search I did)

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 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Feb 2, 2023
@yaira2 yaira2 merged commit 362434d into files-community:main Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing properties private T Service { get; } = Ioc.Default.GetRequiredService<T>() by readonly field
4 participants