Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add async XLinq document/element loading and saving#14774

Merged
sepidehkh merged 1 commit intodotnet:masterfrom
sepidehkh:AddAsyncXLinqLoadSave
Jan 3, 2017
Merged

Add async XLinq document/element loading and saving#14774
sepidehkh merged 1 commit intodotnet:masterfrom
sepidehkh:AddAsyncXLinqLoadSave

Conversation

@sepidehkh
Copy link
Copy Markdown
Contributor

This PR resolves https://github.com/dotnet/corefx/issues/2869. Merges #2436 from future branch into master and exposes the new APIs for netcoreapp1.1.

@danmosemsft @stephentoub @weshaggard

@sepidehkh sepidehkh added area-System.Xml enhancement Product code improvement that does NOT require public API changes/additions labels Dec 30, 2016
@sepidehkh sepidehkh self-assigned this Dec 30, 2016
@sepidehkh sepidehkh force-pushed the AddAsyncXLinqLoadSave branch from 6d3c879 to 1257e2d Compare December 30, 2016 20:36
public override Task WriteToAsync(XmlWriter writer, CancellationToken cancellationToken)
{
if (writer == null)
throw new ArgumentNullException("writer");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: nameof(writer) (I think we switched over to using nameof throughout after these changes were already checked-in to the future branch). Applies to a few other files in this PR.

@@ -0,0 +1,205 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be replaced with the new license header

public XCData(System.Xml.Linq.XCData other) : base(default(string)) { }
public override System.Xml.XmlNodeType NodeType { get { throw null; } }
public override void WriteTo(System.Xml.XmlWriter writer) { }
public override System.Threading.Tasks.Task WriteToAsync(System.Xml.XmlWriter writer, System.Threading.CancellationToken cancellationToken) { throw null; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe all of these new APIs in the contracts need to be surrounded by #if netcoreapp11. See some of the other PRs that contribute to #2869 like #12600 and #13828.

@sepidehkh sepidehkh force-pushed the AddAsyncXLinqLoadSave branch from 1257e2d to 18c59b5 Compare December 30, 2016 23:30
@sepidehkh
Copy link
Copy Markdown
Contributor Author

@justinvp Thanks for your comments. Made according changes.

@sepidehkh sepidehkh force-pushed the AddAsyncXLinqLoadSave branch from 18c59b5 to 318ede2 Compare December 31, 2016 00:06
public static System.Xml.Linq.XDocument Load(System.Xml.XmlReader reader) { throw null; }
public static System.Xml.Linq.XDocument Load(System.Xml.XmlReader reader, System.Xml.Linq.LoadOptions options) { throw null; }
#if netcoreapp11
public static System.Threading.Tasks.Task<System.Xml.Linq.XDocument> LoadAsync(System.IO.Stream stream, System.Xml.Linq.LoadOptions options, System.Threading.CancellationToken cancellationToken) { throw null; }
Copy link
Copy Markdown
Member

@danmoseley danmoseley Jan 3, 2017

Choose a reason for hiding this comment

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

do you know why we don't put 'async' keyword in the ref file? (not your change)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That would create another private type for the state machine that isn't necessary.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Copy Markdown
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Examined the diffs and I believe this is an accurate port. I'll not merge in case one of the other guys want to look.

@sepidehkh
Copy link
Copy Markdown
Contributor Author

Any additional comments?

@stephentoub
Copy link
Copy Markdown
Member

I'll take a look today...

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Looks like a faithful port; thanks.

If you haven't already, you might want to diff the current XLinq code against the XLing code from the future branch to see if anything else changed that could impact this. The async code was derived from the sync code, so if the sync code has changed in the interim (e.g. if anything got modified/augmenting as part of bringing back more APIs), the async code may need to as well. I don't know if there are any such cases, though.

@sepidehkh
Copy link
Copy Markdown
Contributor Author

Thanks for reviewing.

@sepidehkh sepidehkh merged commit b119ec5 into dotnet:master Jan 3, 2017
@karelz karelz modified the milestone: 2.0.0 Jan 3, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…adSave

Add async XLinq document/element loading and saving

Commit migrated from dotnet/corefx@b119ec5
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xml enhancement Product code improvement that does NOT require public API changes/additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge future branch back into master

7 participants