Updating references to NuGet packages #22

SvenAelterman · 2019-10-08T14:07:30Z

I've mainly updated the references to the latest stable versions of everything.
I've also done a few things with Code Analysis to address or suppress rules.

I am open to your feedback and modifications to my PR.

If you're still maintaining this project, I'd appreciate the merge and a release of an updated NuGet package? Thanks!

Updating from fork's origin

Add exclusion for .vs/ folder

Update StyleCop to latest stable

Add stylecop.json and remove exclusion from .gitignore Add GlobalSuppression file to suppress documentation rule

Bouke · 2019-10-10T12:49:51Z

System.Web.Mvc.Expressions/System.Web.Mvc.Expressions.csproj

-    <Reference Include="System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
-      <HintPath>..\packages\Microsoft.AspNet.Mvc.5.2.3\lib\net45\System.Web.Mvc.dll</HintPath>
-      <Private>True</Private>
+    <Reference Include="System.Web.Mvc, Version=5.2.7.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Would it be possible to not reference an explicit version here? So it'll work regardless of what version is actually installed?

Thanks for looking into it. I am not familiar enough with the possible reference styles to answer that question. The other consideration is that even if Visual Studio will be able to locate the reference, when it builds, it's might still bind to the specific version located at compile time.

This SO answer seems to support my thinking: https://stackoverflow.com/questions/24022134/how-exactly-does-the-specific-version-property-of-an-assembly-reference-work-i, but I have not tested it myself.

Actually it's best for a library to use the lowest possible version of it's dependencies and mark the dependency with <SpecificVersion>False</SpecificVersion> (as described in the SO answer @SvenAelterman refers to). So maybe it's better to downgrade to 5.0.0 (basically what @ivaylokenov did in this commit f56c657#diff-64f0574f3b8754e57f68807d79fc91a8). About the green squiggly that's mentioned in issue #21: targeting 5.0.0 MVC and 3.0.0 Razor + WebPages packages doesn't generate it - tested it locally. The detailed build log shows that the 5.2.7 version of MVC is used:

1> Unified primary reference "System.Web.Mvc, Version=5.2.7.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35". 1> Using this version instead of original version "5.0.0.0" in "<BASE-PATH>\My-ASP.NET-MVC-Lambda-Expression-Helpers\System.Web.Mvc.Expressions\bin\Release\System.Web.Mvc.Expressions.dll" because of a binding redirect entry in the file "Web.config".

vladislav-karamfilov · 2019-12-25T15:12:47Z

System.Web.Mvc.Expressions/Internals/MvcExtensions.cs

    {
        private const string ControllerSuffix = "Controller";

+#pragma warning disable SA1306


It's better to make these static readonly instead of suppressing the rule.

vladislav-karamfilov · 2019-12-25T15:13:18Z

System.Web.Mvc.Expressions/Internals/MvcExtensions.cs

        }
    }
-}
+}


We would prefer having a blank line at the end of the file.

vladislav-karamfilov · 2019-12-25T15:18:49Z

System.Web.Mvc.Expressions/GlobalSuppressions.cs

+// Project-level suppressions either have no target or are given 
+// a specific target and scoped to a namespace, type, member, etc.
+
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.DocumentationRules", "SA1600:Elements should be documented", Justification = "<Pending>", Scope = "type", Target = "~T:System.Web.Mvc.Expressions.Internals.MvcExtensions")]


It would be more consistent and better to just add the suppressed rule here: https://github.com/ivaylokenov/ASP.NET-MVC-Lambda-Expression-Helpers/blob/master/System.Web.Mvc.Expressions/System.Web.Mvc.Expressions.ruleset. This way we won't have a new file that needs to be compiled on each project build.

vladislav-karamfilov · 2019-12-25T17:44:33Z

System.Web.Mvc.Expressions/System.Web.Mvc.Expressions.csproj

-    <Reference Include="System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
-      <HintPath>..\packages\Microsoft.AspNet.Mvc.5.2.3\lib\net45\System.Web.Mvc.dll</HintPath>
-      <Private>True</Private>
+    <Reference Include="System.Web.Mvc, Version=5.2.7.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Actually it's best for a library to use the lowest possible version of it's dependencies and mark the dependency with <SpecificVersion>False</SpecificVersion> (as described in the SO answer @SvenAelterman refers to). So maybe it's better to downgrade to 5.0.0 (basically what @ivaylokenov did in this commit f56c657#diff-64f0574f3b8754e57f68807d79fc91a8). About the green squiggly that's mentioned in issue #21: targeting 5.0.0 MVC and 3.0.0 Razor + WebPages packages doesn't generate it - tested it locally. The detailed build log shows that the 5.2.7 version of MVC is used:

1> Unified primary reference "System.Web.Mvc, Version=5.2.7.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35". 1> Using this version instead of original version "5.0.0.0" in "<BASE-PATH>\My-ASP.NET-MVC-Lambda-Expression-Helpers\System.Web.Mvc.Expressions\bin\Release\System.Web.Mvc.Expressions.dll" because of a binding redirect entry in the file "Web.config".

xantari · 2020-07-08T12:06:08Z

Why hasn't this been merged in yet?

SvenAelterman · 2020-07-08T12:10:19Z

I haven't had a chance to update the PR yet as requested by the maintainers. I know it needs to be done though.

vladislav-karamfilov · 2020-08-17T19:47:48Z

I haven't had a chance to update the PR yet as requested by the maintainers. I know it needs to be done though.

Hey, @SvenAelterman . If you are OK, I can make some time during the weekend to address all PR comments so we can close this PR and release a new version.

SvenAelterman · 2020-08-28T23:45:18Z

@vladislav-karamfilov Yes, please make the modifications as you see fit.

vladislav-karamfilov · 2020-09-27T19:03:23Z

@vladislav-karamfilov Yes, please make the modifications as you see fit.

Hey, @SvenAelterman! I finally had the time to address the CR comments. I opened a PR (Sorrell-Solutions#2) to your fork's master and when you merge it, we'll be finally able to merge this PR and release a new NuGet package version. Thank you! :)

Addressed code review comments of PR "Updating references to NuGet packages"

CLAassistant · 2020-09-27T20:50:16Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ SvenAelterman
❌ vladislav-karamfilov
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

SvenAelterman · 2020-09-27T20:50:55Z

@vladislav-karamfilov I've just approved your PR into my forked repo.

SvenAelterman added 4 commits October 8, 2019 08:46

Merge pull request #1 from ivaylokenov/master

47076a3

Updating from fork's origin

Add .vs/ folder exclusion to .gitignore

1b8c61f

Add exclusion for .vs/ folder

Update ASP.NET MVC to 5.2.7; including dependencies

21dd473

Update StyleCop to latest stable

Address some StyleCop warnings

ab06f28

Add stylecop.json and remove exclusion from .gitignore Add GlobalSuppression file to suppress documentation rule

SvenAelterman mentioned this pull request Oct 8, 2019

Assuming Assembly reference when using System.Web.Mvc 5.2.4.0 - 5.2.6.0 #21

Open

Bouke reviewed Oct 10, 2019

View reviewed changes

vladislav-karamfilov suggested changes Dec 25, 2019

View reviewed changes

vladislav-karamfilov added 2 commits September 27, 2020 11:39

Move StyleCop rules suppression to the ruleset file.

cfa3d30

Downgraded to ASP.NET MVC 5.0.0

06c92e8

Merge pull request #2 from vladislav-karamfilov/master

a2be407

Addressed code review comments of PR "Updating references to NuGet packages"

vladislav-karamfilov approved these changes Sep 28, 2020

View reviewed changes

Updating references to NuGet packages #22

Are you sure you want to change the base?

Updating references to NuGet packages #22

Uh oh!

Conversation

SvenAelterman commented Oct 8, 2019

Uh oh!

Bouke Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

SvenAelterman Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

vladislav-karamfilov Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

vladislav-karamfilov Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

vladislav-karamfilov Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

vladislav-karamfilov Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

vladislav-karamfilov Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

xantari commented Jul 8, 2020

Uh oh!

SvenAelterman commented Jul 8, 2020

Uh oh!

vladislav-karamfilov commented Aug 17, 2020

Uh oh!

SvenAelterman commented Aug 28, 2020

Uh oh!

vladislav-karamfilov commented Sep 27, 2020

Uh oh!

CLAassistant commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SvenAelterman commented Sep 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Sep 27, 2020 •

edited

Loading