search

alexbeletsky / elmah-mvc

Painless integration of ELMAH into ASP.NET MVC application
http://nuget.org/packages/Elmah.MVC
Apache License 2.0
266 stars 61 forks source link

FxCop and StyleCop issues in ElmahController and AdminAreaRegistration #4

Closed CallumHibbert closed 12 years ago

CallumHibbert commented 12 years ago

Hello,

Nice product but a couple of minor annoyances. The code in ElmahController and AdminAreaRegistration generates alot of FxCop and StyleCop errors (when using those products with settings out of the box).

Could you make the generated code a bit more compliant to those standards?

Some FxCop issues:

Some StyleCop issues:

Thanks,

Callum

alexbeletsky commented 12 years ago

Thanks for that! I'll check it with StyleCop and FxCop. It would be nice if you attach your existing reports to this issue.

alexbeletsky commented 12 years ago

Decieded to close that for several reasons:

  1. FxCop issues are not critical, requires not changes.
  2. StyleCop issues has not sense to fix, since different projects have different configurations, could not create idea source code file that would fulfill all requirements anyway.
  3. ELMAH.MVC are deployed as source code, you can re-format, appy changes whatever you want, to meet own standards/requirement.
CallumHibbert commented 12 years ago

Some things to consider.

You say that FxCop issues are not critical but we treat our FxCop warnings as errors. So when we add Elmah MVC we get 16 errors which fail the compilation. Likewise with StyleCop, we treat the warnings as errors. So between the two we get a lot of critical errors.

These issues create friction when we add the product to our solution, a failed compile for 16 FxCop errors, fixing or suppressing the FxCop errors, a failed build due to StyleCop errors and then re-formatting for StyleCop. This is frustrating as, aside from the "treat warnings as errors" options, we are using out of the box settings. I agree that different projects have different standards but we are using the out of the box style (what I would consider industry standard and best practice). No exotic coding styles. This brings me to the coding style for the ELMAH MVC code, which is Java-esque and a long way from standard C# style. I would expect a tool that wants to be widely used to baseline to the closest thing we have to an industry standard (the StyleCop out of the box style).

These issues are a bit of a departure from the intention of NuGet - frictionless dependency management.

I hope you re-consider your decision, a good solution would be:

The "pre-suppression" approach would mean adding these statements into the source code:

[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1062:Validate arguments of public methods", MessageId = "0", Scope = "member", Target = "[project namespace].Areas.Admin.AdminAreaRegistration.#RegisterArea(System.Web.Mvc.AreaRegistrationContext)")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1062:Validate arguments of public methods", MessageId = "0", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahResult.#ExecuteResult(System.Web.Mvc.ControllerContext)")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object)", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahResult.#FilePath(System.Web.Mvc.ControllerContext)")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1702:CompoundWordsShouldBeCasedCorrectly", MessageId = "Stylesheet", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Stylesheet()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Elmah", Scope = "type", Target = "[project namespace].Areas.Admin.Controllers.ElmahController")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Rss", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#DigestRss()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Rss", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Rss()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#About()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Detail()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#DigestRss()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Download()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Index()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Json()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Rss()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Stylesheet()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Scope = "member", Target = "[project namespace].Areas.Admin.Controllers.ElmahController.#Xml()")]

But it would be better to validate the arguments, mark the relevant members as static etc.

alexbeletsky commented 12 years ago

If you already have all those issue fixed and I hope it is doable just with ReSharper, would you consider possibility to make a pull request ?