Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Update creating-app-with-plugin-support.md #43679
base: main
Are you sure you want to change the base?
Update creating-app-with-plugin-support.md #43679
Changes from 1 commit
22a5538
c25c22a
0457084
b8fda16
e5dce31
735872d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "namespace" in this sentence refer to? I don't think the C# namespace is what is implied here and using that term is likely to cause more confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dare I propose "assembly context"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Namespace meaning that type names do not necessarily resolve to the same types. I really don't know a better runtime term for that.
That is, Newtonsoft.JSON.JsonObject in one context does not necessarily refer to the same type as Newtonsoft.JSON.JsonObject in another ALC.
On the one hand this doesn't feel like a very well-known term. On the other hand, should it be? I think this provides the best mental model I have when thinking about ALCs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. "Assembly context", as proposed in #43679 (comment), is what I would use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use "isolated" in number of other assembly load context docs.
For example, https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/understanding-assemblyloadcontext#what-is-the-assemblyloadcontext says "creates an isolated context for loading code and its dependencies". Fix those too for consistency?
Check failure on line 19 in docs/core/tutorials/creating-app-with-plugin-support.md
GitHub Actions / lint
Trailing spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we update versions here? 5 is EOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we update everywhere that we mention a version in the docs on a regular basis? I think this is fine because it says "or a newer version."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typically you don't want to have EOL versions like this. the warning later on says that the code shows .NET 5 code but the that the feature was introduced in 3.0. I think saying when it was introduced is fine but IMHO the article should say .NET 8 or later versions and the code should work in 8+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. I think that just puts us in the same position WRT to EOL versions when .NET 8 eventually goes out of support. The only future-proof option is to avoid recommending any specific version for installation. I don't know what we do about samples that have to contain a TFM. It seems unavoidable that we will end up with EOL versions in those places.
I think we need to agree on one of two approaches: either we're OK with older versions appearing in docs/samples, or someone is put in charge of sweeping all the docs for EOL versions every time a version goes out of support. Piecemeal approaches will not leave us in a good state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the text about which SDK this is supported in entirely. I don't think we have a responsibility for explaining that a feature doesn't work in an SDK that has been EOL for quite some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. For this case I think we can just remove mention of the specific version. For samples that have to contain some "netxx" we need to come up with a plan there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that