While true, it is not really an issue as the classes that use
AssemblyUtility only scan dlls and exes. The text file will never get
loaded or indicated as a reference.

I don't have a big concern but I don't think was needed.
On Sat, Feb 7, 2015 at 2:38 PM Adam Ralph notifications@github.com wrote:

This is a refactoring of the changes in #923
#923 which fixed #922
#922.

Whilst that PR fixed the bug and allowed us to ship 0.13.2, I was quite
uncomfortable leaving AssemblyUtility.IsManagedAssembly() returning true
whenever the the path passed to it isn't rooted and doesn't end in 'dll' or
'exe'. For instance, I could pass "ThisDoesNotExist.txt" into it and it
would return true.

In this PR I've reverted AssemblyUtility and instead changed
PackageAssemblyResolver to filter out unmanaged assemblies from the list
returned from IPackageObject.GetCompatibleDlls() before concatenating
them with IPackageObject.FrameworkAssemblies.

IMO the quickest way to review this PR is one commit at a time.

You can view, comment on, or merge this pull request online at:

#926
Commit Summary

• refactor: simplify conditional, fix whitespace, remove and sort
  usings
• green: add scenario
  Packages.PackageContainsAFrameworkAssemblyReference
• red: revert 923 AssemblyUtility changes
• green: implement scenario
  Packages.PackageContainsAFrameworkAssemblyReference
• refactor: AssemblyResolver and PackageAssemblyResolver internals

File Changes

• M ScriptCs.sln
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-0 (5)
• M src/ScriptCs.Core/AssemblyResolver.cs
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-1 (62)
• M src/ScriptCs.Core/AssemblyUtility.cs
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-2 (13)
• M src/ScriptCs.Core/PackageAssemblyResolver.cs
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-3 (91)
• M test/ScriptCs.Core.Tests/AssemblyUtilityTests.cs
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-4 (16)
• M test/ScriptCs.Core.Tests/PackageAssemblyResolverTests.cs
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-5 (51)
• A test/ScriptCs.Tests.Acceptance/Packages.cs
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-6 (31)
• M test/ScriptCs.Tests.Acceptance/ScriptCs.Tests.Acceptance.csproj
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-7 (4)
• A
  test/ScriptCs.Tests.Acceptance/Support/Gallery/FrameworkAssemblyReferencer.1.0.0.nupkg
  https://github.com/scriptcs/scriptcs/pull/926/files#diff-8 (0)

Patch Links:

• https://github.com/scriptcs/scriptcs/pull/926.patch
• https://github.com/scriptcs/scriptcs/pull/926.diff

—
Reply to this email directly or view it on GitHub
#926.

It's true that the original fix was fine given all the calls made to AssemblyUtility.IsManagedAssembly from the scriptcs codebase, but the method is public and packaged in core, so I think it's important that it conveys the correct semantics. In fact, even if it was internal, I'd still consider this important.

@scriptcs/core does anyone fancy merging this? Are there any concerns? It should be quite quick to review if you take one commit at a time.

@adamralph 👍

woot! thanks @khellang