-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Code Quality: Reorganze namespaces #11552
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
Conversation
…amespaces for the same purpose
Ready to review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@onein528 could you look into the conflicts? |
…8/Files into 5bfa/organize-viewmodels
@d2dyno1 are you okay with these changes? |
@@ -264,12 +264,12 @@ public DynamicDialogButtons DynamicButtonsEnabled | |||
/// </summary> | |||
public Action HideDialog { get; set; } | |||
|
|||
private Action<DynamicDialogViewModel, ContentDialogButtonClickEventArgs> primaryButtonAction; | |||
private Action<DynamicViewModel, ContentDialogButtonClickEventArgs> primaryButtonAction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be confusing when using multiple namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well not include Settings, Properties, Page like AboutSettingsViewModel. This is not accepted(AboutViewModel will be okay).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this approach. While I'd agree to shorten longer class names, in this example, the full name is actually necessary.
Someone looking at the code might question themselves: "DynamicViewModel of what? How is it dynamic? Oh, it's for a dialog..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how about Settings.AboutViewModel? I think we should avoid duplication because you can hover on them and look for where it from and what it represents if they have the XAML comment. That's why I changed Properties.PropertiesGeneral to Properties.GeneralPage(although Page is necessary, they didn't have it) and I added ViewModels.UserControls.* instead of putting directly UserControl's ViewModel into ViewModels.*
Description
Reorganize namespaces. For more information, see Details of Changes
Motivation and Context
I found that there're a lot of namespaces that need to be reorganized and there're namespaces that do not need to be separated and I merged them into the same.
Details of Changes
Files.App.Controllers.IJson
Files.App.ValueConverters
Files.App.Converters
Files.App.AppModels
Files.App.DataModels
Files.App.CommandLine
Files.Backend.CommandLine
UserControls/Pane/PreviewPane
UserControls/PreviewPane
Files.App.ViewModels.*
)Files.App.ViewModels.UserControls
Validation