search

aspnet / live.asp.net

Code for live.asp.net, which hosts the ASP.NET Community Stand-up
https://live.asp.net/
MIT License
289 stars 114 forks source link

Simplify IShowService #107

Closed hishamco closed 7 years ago

hishamco commented 7 years ago

@DamianEdwards your feedback

hishamco commented 7 years ago

@DamianEdwards is this fine for you?

DamianEdwards commented 7 years ago

No, I don't like using IHttpContextAccessor.

hishamco commented 7 years ago

Strange!! Is there any reason to avoid using IHttpContextAccessor?

DamianEdwards commented 7 years ago

The service doesn't depend on IHttpContext, it just needs the user.

hishamco commented 7 years ago

I injected the IHttpContextAccessor to access the User, which is similar to what you did here https://github.com/aspnet/live.asp.net/blob/92d0385e7d1e4e052deba4bbaaf13bacbf9d2d7e/src/live.asp.net/Controllers/HomeController.cs#L27, furthermore I'm not sure why it depends on the ClaimsPrincipals while the intend is to get the recorded shows

DamianEdwards commented 7 years ago

In the example you gave, I grabbed the user from the class I'm already in, so that's not the same. I need the user because I don't allow non-authenticated to bypass the cache.

hishamco commented 7 years ago

It uses ControllerBase.User https://github.com/aspnet/Mvc/blob/1dd1d49321eab65431e9c4b0465d7d9f2b23dbdf/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs#L230, which is the similar thing that I access it via DI

DamianEdwards commented 7 years ago

It really isn't :smile:

hishamco commented 7 years ago

I got confused 😄