aspnet / Templates

This repo is OBSOLETE - please see the README file for information
Other
150 stars 57 forks source link

Templates to enforce `using` #639

Closed IEvangelist closed 8 years ago

IEvangelist commented 8 years ago

Since the IWebHost implements IDisposable, let's make the templates enforce best practices.

dnfclas commented 8 years ago

Hi @IEvangelist, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

phenning commented 8 years ago

@davidfowl thoughts?

peterblazejewicz commented 8 years ago

2c: The best practise documentation (and sample code) usually are simple, short and explicit: https://msdn.microsoft.com/en-us/library/3bwa4xa9(v=vs.110).aspx e.g.:

using (StreamReader s = new StreamReader("File1.txt")) {

but not:

using(var a = new b()
  .UseFeatureC()
  .UseFeatureD()
  .UseAsFeatureE()
  .Build())

In such complex code there is no obvious hint which method returns IDisposable: contructor, extension methods, build method? It obviates possible gain from changing a code IMO. The linked docs states that there are two ways to handle IDisposable BTW (depending on contex, preferences, etc)

@IEvangelist white space should not to be changed, see: https://github.com/aspnet/Templates/pull/639/files?w=1 @phenning https://github.com/aspnet/Templates/issues/226

IEvangelist commented 8 years ago

@peterblazejewicz So would you prefer to have a try / finally? I guess my point is that I want the IDisposable to be honored. It belongs in either a using or in a try / finally.

peterblazejewicz commented 8 years ago

No, I don't have preferences here (I use a lot of using at work though). Let's wait for @davidfowl 💬

davidfowl commented 8 years ago

We debated this a bit and moved away from it. Run disposed the appropriate things.

IEvangelist commented 8 years ago

@davidfowl I see why now. It certainly is not obvious from the templates -- you have to go looking at the implementation of Run, which is an extension method, which internally wraps its usage in a using.

davidfowl commented 8 years ago

Yep! It's a bit cleaner to look at since wrapping that many lines of code in a using is pretty awful.

dnfclas commented 8 years ago

@IEvangelist, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, DNFBOT;

dnfclas commented 8 years ago

@IEvangelist, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, DNFBOT;