Files icon indicating copy to clipboard operation
Files copied to clipboard

Code Quality: Clean up Files.Actions phase 1

Open 0x5bfa opened this issue 2 years ago • 6 comments

Code Quality: Clean up Files.Actions

Motivation & Context

  • Improved readability

Warning
Please do not review code lines that I did not changed. I cannot make more diff. If there're more codes that are preferred to change, I will do that and mark that PR as the final one for that stage.

PR Checklist

  • [x] Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers. Related #4180
  • [x] Did you build the app and test your changes?
  • [ ] Did you check for accessibility? You can use Accessibility Insights for this.
  • [ ] Did you remove any strings from the en-us resource file?
    • [ ] Did you search the solution to see if the string is still being used?
  • [ ] Did you implement any design changes to an existing feature?
    • [ ] Was this change approved?
  • [ ] Are there any other steps that were used to validate these changes?

Screenshots

None

0x5bfa avatar Jun 21 '23 14:06 0x5bfa

You should consider opening a different PR to refactor the initialization of context in most actions.

How come should I? I think it’s fine so far. Probably I will split up phase 1 PR into multiple PRs, so reviewers can review quickly and easily.

0x5bfa avatar Jun 22 '23 23:06 0x5bfa

How come should I? I think it’s fine so far.

There are a lot of Actions which use private readonly IContentApplicationContext context = Ioc.Default.GetRequired<IContentPageContext>();

I think the assignment should be moved to the constructor

ferrariofilippo avatar Jun 23 '23 14:06 ferrariofilippo

I think the assignment should be moved to the constructor

All of them were moved.

0x5bfa avatar Jun 23 '23 15:06 0x5bfa

Ready for final review.

0x5bfa avatar Jun 23 '23 15:06 0x5bfa

@yaira2 Ready to merge?

0x5bfa avatar Jun 28 '23 02:06 0x5bfa

@0x5bfa this looks like a big change, I'd like to get a second review before merging.

yaira2 avatar Jun 28 '23 03:06 yaira2

Should I have split up into like 3 PRs? I can do that instead of fixing conflicts and waiting reviews.

0x5bfa avatar Jul 05 '23 02:07 0x5bfa

We can do them together

yaira2 avatar Jul 05 '23 03:07 yaira2

@0x5bfa besides for formatting, were there any other changes?

yaira2 avatar Jul 06 '23 02:07 yaira2

Change overview

  • Moved initializers to the constructors (i.e. dependency injection initializings)
  • Improved formatting
  • Updated return values of Execute(); (i.e. removing async for non-async) You can see this changing in ferrariofilippo's reviews

0x5bfa avatar Jul 06 '23 02:07 0x5bfa