-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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 core/porting/libraries #2467
Update core/porting/libraries #2467
Conversation
😅Finally! @mairaw @cartermp It probably needs a little work. For sure, there are a few spots for special attention because I either wasn't sure or concerned:
|
docs/core/porting/libraries.md
Outdated
|
||
Read more in [.NET Platform Standard Library](../../standard/net-standard.md). | ||
Just because an API or technology isn't currently implemented doesn't imply it's intentionally unsupported. Feel free to file an issue in the [dotnet/corefx repository issues](https://github.com/dotnet/corefx/issues) at GitHub to ask for specific APIs and technologies. [Porting requests in the issues](https://github.com/dotnet/corefx/labels/port-to-core) are usually marked with the `port-to-core` label. |
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.
[Porting requests in the issues](https://github.com/dotnet/corefx/labels/port-to-core) are usually marked with the `port-to-core` label.
Why is the first part of the sentence linked, instead of the more obvious "the port-to-core
label". That way, it's clear where the link leads, especially considering the "usually".
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.
My intent was to avoid the the link looking like it lands on content that describes the label; it goes to the issues that are marked port-to-core
. "Porting requests in the issues" is what the landing page provides if they click the link.
However, I do think this sentence is blown (as you say: especially considering the "usually"): If the link goes to port-to-core
tagged issues, then "usually" doesn't work here.
I'm going to to refactor this sentence on the next commit.
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 looked this over. Issues for porting requests should be marked port-to-core
, and there's no point saying that it should "usually" be the case. I can't tell the reader how to find the other issues that aren't marked port-to-core
. The simplest way to deal with this is just to remove the word "usually." That makes the sentence a true statement (even if the request was generated internally), and the link provides the content described by the text: "Porting requests in the issues" describes the landing page content. I'm open to suggestions tho, so if you can think of a better way to express the same thought, I'm all 👂👂👂👂👂👂.
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, this looks good to me. I don't have a better way to express it.
docs/core/porting/libraries.md
Outdated
|
||
### App Domains | ||
For code isolation, we recommend separate processes or using containers as an alternative. For the dynamic loading of assemblies, we recommend the new <xref:System.Runtime.Loader.AssemblyLoadContext> class. Information (such as the name and base directory) is provided by APIs on other types, for instance `AppContext.BaseDirectory`. Some scenarios, such as getting the list of loaded assemblies, are unsupported, as they're inherently fragile. |
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.
The examples of AppDomain.BaseDirectory
and AppDomain.GetAssemblies()
are coming back with .Net Core 2.0. Is it okay that this document is going to again become outdated soon?
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 don't have the ability to deal cleanly with versions just yet. I can add "in 1.0" and "for 2.0" language tho. I'm going to ping the CoreFX cats for a word on this, since this is really their content.
docs/core/porting/libraries.md
Outdated
|
||
### App Domains | ||
For code isolation, we recommend separate processes or using containers as an alternative. For the dynamic loading of assemblies, we recommend the new <xref:System.Runtime.Loader.AssemblyLoadContext> class. Information (such as the name and base directory) is provided by APIs on other types, for instance `AppContext.BaseDirectory`. Some scenarios, such as getting the list of loaded assemblies, are unsupported, as they're inherently fragile. |
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.
Why is AppContext.BaseDirectory
code and not xref?
docs/core/porting/libraries.md
Outdated
|
||
AppDomains can be used for different purposes on the .NET Framework. For code isolation, we recommend separate processes and/or containers as an alternative. For dynamic loading of assemblies, we recommend the new @System.Runtime.Loader.AssemblyLoadContext class. | ||
To make code migration from .NET Framework easier, we've exposed some of the <xref:System.AppDomain> API surface in .NET Core. Some of the APIs function normally (for example, <xref:System.AppDomain.UnhandledException?displayProperty=fullName>), some of them do nothing (for example, <xref:System.AppDomain.SetCachePath%2A>), and some of them throw <xref:System.PlatformNotSupportedException> (for example, <xref:System.AppDomain.CreateDomain%2A>). |
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'm confused, as far as I can tell, this paragraph describes the situation as of .Net Core 2.0, but the previous one seems to be about .Net Core 1.x.
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.
@svick Yes, that's correct ... 2.0. I'm going to add "for .NET Core 2.0" language.
@karelz @weshaggard Can you suggest the cleanest split between 1.0 and 2.0 content here? This content was taken (shamelessly stolen actually 😜) from your CoreFX porting doc.
docs/core/porting/libraries.md
Outdated
* @System.Runtime.Serialization.DataContractSerializer for both XML and JSON | ||
* @System.Xml.Serialization.XmlSerializer for XML | ||
* [protobuf-net](https://github.com/mgravell/protobuf-net) for Protocol Buffers | ||
.NET Remoting, which is transparent remote procedure calls, was identified as a problematic architecture. It's used for cross-AppDomain communication, which is no longer supported. Also, Remoting requires runtime support, which is expensive to maintain. For these reasons, .NET Remoting isn't supported on .NET Core. |
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.
.NET Remoting, which is transparent remote procedure calls, …
I think the grammar doesn't quite work here.
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.
There's nothing wrong with a non-essential clause after a noun. However, I don't care for "which is" either. Let's drop the non-essential clause.
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 meant that ".Net Remoting is calls" doesn't sound like correct grammar to me.
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.
You mean this? ......
.NET Remoting, which is transparent remote procedure calls, was identified as a problematic architecture.
... "which is transparent remote procedure calls" is a subordinate, non-essential clause (has a subject and a predicate but doesn't make sense without an object) describing ".NET Remoting" and set off by a comma before and after. Technically, it's ok. I think it sounds like CRAP 😄 lol. Best thing to do here is 🔪 cut that out. If the reader needs to see what it means, they can search it (or we could link to .NET Remoting if you think that's a better solution).
docs/core/porting/libraries.md
Outdated
|
||
### Sandboxes | ||
Across machines, use a network-based solution as an alternative. Preferably, use a low-overhead plain text protocol, such as HTTP. The [Kestrel web server](https://docs.microsoft.com/aspnet/core/fundamentals/servers/kestrel), the web server used by ASP.NET Core, is an option here. Consider <xref:System.Net.Sockets>. For more options, see [.NET Open Source Developer Projects: Messaging](https://github.com/Microsoft/dotnet/blob/master/dotnet-developer-projects.md#messaging). |
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.
The "Consider Sockets." sentence sounds out of place to me here.
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'll expand the sentence to make it more "conversational," but that's a guidance they provided in the CoreFX topic, so I'm inclined to leave it here as an option for the reader to explore.
docs/core/porting/libraries.md
Outdated
## Retargeting your .NET Framework Code to .NET Framework 4.6.2 | ||
|
||
If your code is not targeting .NET Framework 4.6.2, it's recommended that you retarget. This ensures that you can use the latest API alternatives for cases where the .NET Standard can't support existing APIs. | ||
If your code isn't targeting .NET Framework 4.6.2, we recommended that you retarget. This ensures the availability of the latest API alternatives for cases where the .NET Standard doesn't support existing APIs. |
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.
Is this suggesting to use 4.6.2 or newer, 4.6.2 or older, or always exactly 4.6.2? I think that should be clarified.
@svick 👍 Cool ..... I'll get back to you soon. I'm on a priority work assignment for a few days and hope to have it squared away in a few days. |
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.
Overall this looks great. @guardrex I left a few comments. I'm also asking @richlander or another PM to review and make sure our guidance is accurate.
docs/core/porting/libraries.md
Outdated
|
||
Read more in [.NET Platform Standard Library](../../standard/net-standard.md). | ||
Just because an API or technology isn't currently implemented doesn't imply it's intentionally unsupported. Feel free to file an issue in the [dotnet/corefx repository issues](https://github.com/dotnet/corefx/issues) at GitHub to ask for specific APIs and technologies. [Porting requests in the issues](https://github.com/dotnet/corefx/labels/port-to-core) are marked with the `port-to-core` label. |
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 "Feel free to" from the second sentence.
docs/core/porting/libraries.md
Outdated
|
||
AppDomains can be used for different purposes on the .NET Framework. For code isolation, we recommend separate processes and/or containers as an alternative. For dynamic loading of assemblies, we recommend the new @System.Runtime.Loader.AssemblyLoadContext class. | ||
To make code migration from .NET Framework easier, we've exposed some of the <xref:System.AppDomain> API surface in .NET Core. Some of the APIs function normally (for example, <xref:System.AppDomain.UnhandledException?displayProperty=fullName>), some of them do nothing (for example, <xref:System.AppDomain.SetCachePath%2A>), and some of them throw <xref:System.PlatformNotSupportedException> (for example, <xref:System.AppDomain.CreateDomain%2A>). |
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.
You should add a sentence about where to find which classes and APIs take which action (even if it is the obvious "check the types you use for details".)
docs/core/porting/libraries.md
Outdated
* @System.Runtime.Serialization.DataContractSerializer for both XML and JSON | ||
* @System.Xml.Serialization.XmlSerializer for XML | ||
* [protobuf-net](https://github.com/mgravell/protobuf-net) for Protocol Buffers | ||
.NET Remoting was identified as a problematic architecture. It's used for cross-AppDomain communication, which is no longer supported. Also, Remoting requires runtime support, which is expensive to maintain. For these reasons, .NET Remoting isn't supported on .NET Core. |
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.
For consistency, this should add the sentence "we don't plan on adding support in the future" as you did above, if it applies.
@BillWagner For |
That is correct. I was looking for a statement to check each API for specifics. Also, we now have merge conflicts. 😭 Can you rebase to address those? |
@BillWagner I'm a little confused on the "check each API" part of ...
That section only pertains to
Are you saying "was looking" in the sense that the update did the trick, or do you mean "was looking" and it still needs work? |
@guardrex The changes you've made addressed the concerns I had. When I read "some methods do nothing and some throw exceptions", I need to know where to find which do which action to be productive. By pointing readers to that location, we've anticipated that question and given an answer. |
Just heard (and then looked at the source to confirm) from @[DamianEdwards] on the 3/28 Community Standup (31:13) that obtaining a list of loaded assemblies was restored since this topic was originally written. I've removed the offending line on the last commit. [EDIT] Looking further: @BillWagner It prob needs another change. I don't like this language (at least written this way) ...
... due to the presence of ... and Since we say to look at the API and provide a nice, low-barrier link to it, how about we strike that line, too? |
Ok, I'm done ...... unless anyone has something. It's about time for this chick 🐤 to fly the coop. |
Also waiting on TFS#: 1023634 CI Build reports valid API xref links as invalidInvisible zero-width spaces! Arrrrrrrr!
Fixes #2255