Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RegisterItemsFromAssembly - Include assemblies from nuget packages (Strict) #2519

Merged
merged 2 commits into from
Jan 20, 2018
Merged

RegisterItemsFromAssembly - Include assemblies from nuget packages (Strict) #2519

merged 2 commits into from
Jan 20, 2018

Conversation

snakefoot
Copy link
Contributor

Less aggressive, so NLog.Extensions.Logging.Tests doesn't get marked as hidden assembly.

@snakefoot snakefoot force-pushed the IAppDomainGetAssemblies branch 3 times, most recently from c435d3c to 883fae2 Compare January 15, 2018 23:08
@snakefoot snakefoot mentioned this pull request Jan 15, 2018
Merged
@codecov
Copy link

codecov bot commented Jan 15, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@f214ffd). Click here to learn what that means.
The diff coverage is 11%.

@@           Coverage Diff            @@
##             master   #2519   +/-   ##
========================================
  Coverage          ?     81%           
========================================
  Files             ?     323           
  Lines             ?   23380           
  Branches          ?    2924           
========================================
  Hits              ?   19048           
  Misses            ?    3559           
  Partials          ?     773

304NotModified
304NotModified reviewed Jan 15, 2018
View reviewed changes
src/NLog/Config/ConfigurationItemFactory.cs Outdated
@@ -427,18 +428,21 @@ private static ConfigurationItemFactory BuildDefaultFactory()
var allAssemblies = LogFactory.CurrentAppDomain.GetAssemblies();
foreach (var assembly in allAssemblies)
{
if (assembly.FullName.StartsWith("NLog.", StringComparison.OrdinalIgnoreCase))
if (assembly.FullName.StartsWith("NLog", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less strict? This would match nlogger.dll?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It matches the current: Directory.GetFiles(assemblyLocation, "NLog*.dll")

It is the hidden assembly logic, that is more strict.

Copy link
Contributor Author

@snakefoot snakefoot Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I don't mind reverting it back to NLog. (But then we should consider also fixing the GetFiles-call (Maybe in NLog ver. 5.0)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the getfiles is backwards compatible, isn't? Would prefer that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it will suddenly not load NLogSplunkTarget.dll (Made Up)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you have convinced me, matching on filename is different from matching on namespace. Have added the dot back, so it remains strict.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the filter start with "nlog." was already there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and now it is back again :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

src/NLog/Config/ConfigurationItemFactory.cs
|| assembly.FullName.StartsWith("Microsoft.Extensions.Logging,", StringComparison.OrdinalIgnoreCase)
|| assembly.FullName.StartsWith("Microsoft.Extensions.Logging.Abstractions,", StringComparison.OrdinalIgnoreCase)
|| assembly.FullName.StartsWith("Microsoft.Extensions.Logging.Filter,", StringComparison.OrdinalIgnoreCase)
|| assembly.FullName.StartsWith("Microsoft.Logging,", StringComparison.OrdinalIgnoreCase))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the duplicates, it's a "starts with"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice the ,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks, hardly to see on mobile.

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 20, 2018
@304NotModified 304NotModified added this to the 4.5 rc5 milestone Jan 20, 2018
@304NotModified 304NotModified merged commit 84174bd into NLog:master Jan 20, 2018
@snakefoot snakefoot deleted the IAppDomainGetAssemblies branch January 29, 2018 18:11
@snakefoot snakefoot mentioned this pull request Feb 10, 2018
Closed
@snakefoot snakefoot modified the milestones: 4.5 rc5, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@304NotModified 304NotModified 304NotModified approved these changes

Assignees
No one assigned
Labels
enhancement Improvement on existing feature
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@snakefoot @304NotModified