Skip to content

Instantly share code, notes, and snippets.

@grokys

grokys/GHfVS API review notes.md

Created May 3, 2017 11:56
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save grokys/46476297929e0110fee84622c05626e7 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save grokys/46476297929e0110fee84622c05626e7 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
GHfVS API review notes.md

GitHub.VisualStudio API Review Notes

We have 2 different places for views

  • GitHub.VisualStudio.Views
  • GitHub.VisualStudio.UI.Views

We have 3 different places for controls

  • GitHub.UI.Reactive.Controls
  • GitHub.VisualStudio.UI.Controls
  • GitHub.VisualStudio.UI.Views.Controls

GitHub.VisualStudio.UI.Views doesn't correspond to file location: warning

Exports

We have 2 different exports assemblies:

  • GitHub.Exports
  • GitHub.Exports.Reactive

IObservable<> is in the BCL: as long as we use this in the interfaces, can't we have them all in 1 assembly?

Service Providers

  • What is the difference between GetService and TryGetService in IServiceProviderExtensions? Documentation says that TryGetService is a "Safe variant of GetService that doesn't throw exceptions if the service is not found." except AFAICS GetService just calls TryGetService, casts the result and doesn't throw anyway.
  • What is the difference between TryGetService and GetExportedValue in IServiceProviderExtensions? They appear to be the same.
  • Is IUIProvider misnamed? For example here we use UIProvider to get an IConnection, but IConnection has nothing to do with UI as such.
  • I think understand what the GetService, TryGetService etc methods on IServiceProviderExtensions are trying to do: if the service provider is an IUIProvider, then call GetService on it, otherwise get an instance of IUIProvider from the service provider and call GetService on that. However I feel this is more confusing than helpful:
    • If you call provider.GetService(type(T)) then this mechanism will be bypassed, meaning that it will give different results to provider.GetService<T>()
    • It masks the fast that there are different service providers for different types of resources. IMO it would be clearer to see:
  • Confusing that Substitutes.ServiceProvider doesn't return the same object each time, instead creates a new one.
  • What is IExportFactoryProvider.UIControllerFactory for? Can't you just request a UIController from UIProvider as it's marked NonShared.

Use MVVM

ViewModels should not reference views. For example, GitHubPaneViewModel.Control.

ViewModels should reference other view models and it's the responsibility of the view layer to instantiate views for bound view models.

GitHubPaneViewModel is the worst offender; so much that it needs to be defined under Views rather than ViewModels! For example, the view should set up the toolbar buttons, the ViewModel should just define commands for them and then they should be bound together (even if its not possible to bind using XAML bindings)

ITeamExplorerServiceHolder

Implements a weird kinda-but-not-really IObservable pattern. Use Observables.

SharedDictionaryManager

There are 3 implementations of this

Models

Why do our models (e.g. PullRequestModel) need interfaces? They have no logic.

Misc

  • GitHubPaneViewModel is in GitHub.VisualStudio.UI.Views
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment