Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Decouple ApplicationName from any assembly names #1180

Closed
wants to merge 10 commits into from

Conversation

khellang
Copy link
Contributor

@khellang khellang commented Aug 20, 2017

Here's a proposal to fix a couple of issues...

This PR effectively stops using StartupAssembly to check whether to scan for an IStartup type. Instead it always sets StartupAssembly when calling Configure, UseStartup or UseStartupAssembly (new), and in addition, it sets a new FindStartupType key when you need to scan for a startup type.

If StartupAssembly hasn't been set (no method above has been called), it falls back to the entry assembly name. This fixes #1137.

Instead of using ApplicationName in HostingStartupAssemblies, it now uses StartupAssembly.

This means that we no longer use ApplicationName for any assembly loading or scanning, it's simply just a string that you can set to whatever you want. This fixes #1179.

If an ApplicationName hasn't been provided (by the user), it falls back to StartupAssembly (which could've already fallen back to the entry assembly name).

@dnfclas
Copy link

dnfclas commented Aug 20, 2017

@khellang,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

I don't see the point of this change if consumers of IHostingEnvironment still assume ApplicationName is an assembly name. Save this for 3.0 when we can change both things together.

@khellang khellang force-pushed the fix-application-name branch from 9aa43a6 to 3599d25 Compare August 20, 2017 21:38
@khellang
Copy link
Contributor Author

khellang commented Aug 20, 2017

I don't see the point of this change if consumers of IHostingEnvironment still assume ApplicationName is an assembly name. Save this for 3.0 when we can change both things together.

Haven't we already established that you can't confidently assume that ApplicationName is an assembly name? We've already seen several reports that people are using ApplicationName for other things. And you can't really blame them given the unfortunate naming and documentation. Since the IHostingEnvironment property is settable, there's no guarantee that it'll be an assembly name at all. IMO, any assumption that it's an assembly name is a bug.

@davidfowl
Copy link
Member

Haven't we already established that you can't confidently assume that ApplicationName is an assembly name?

No, if it's set, it's an assembly name. Anyone that changes it to something else will soon learn that this assumption is squarely baked in at multiple levels.

We've already seen several reports that people are using ApplicationName for other things. And you can't really blame them given the unfortunate naming and documentation. Since the IHostingEnvironment property is settable, there's no guarantee that it'll be an assembly name at all. IMO, any assumption that it's an assembly name is a bug.

The problem is this fixes nothing. Several components use ApplicationName as an assembly name like MVC. This change would have been great to do before 2.0 but now I don't see what you've done besides given people false hope that they can now use ApplicationName when infact it will still break.

@davidfowl
Copy link
Member

I'd prefer if we introduced a new property in the next major version that was something like FriendlyName (like appdomain has)

@khellang
Copy link
Contributor Author

Alright. You've convinced me. I guess it's too late to do anything about it 😢

@khellang khellang closed this Aug 20, 2017
@khellang khellang deleted the fix-application-name branch August 20, 2017 22:47
@davidfowl
Copy link
Member

v3!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants