Love it

First of all, I want to say thank-you for your contribution. This looks like a lot of great work and is the first contribution that we've had with API additions!

At the moment, we're right in the middle of bringing our API review process out in the open - and we're not immediately ready to take on this change. Over the next couple of weeks we'll work with you and use this PR as the guinea pig of the process.

In the meantime, we'll provide some feedback on the pull request itself.

Sounds great, lets figure out this process. I'll do my best to address feedback.

I really like the idea of these new async APIs. I'll do my best to push it :) Good job!

Verification is admittedly difficult without a test harness for the project.

I (minimally) checked myself by using the monster JMDict file, loading/saving both in 4.5.2 and with the new sync/async variants to verify the output was identical.

Has anyone had a chance to look at this yet?

Apologies, we haven't forgotten about this. There's two things that we're working on that are preventing us from taking this right at this moment:

1. API review process. @terrajobst has written a proposal and will be publishing over the next week or so. We'll be taking you through you that soon.

2. Branching. We're trying to figure out our branching structure at the moment. Currently we have only a single branch that represents what we're shipping for the next update of .NET Core. Clearly this isn't scalable for the product, and we're going to creating branches for future work so PR's that have API changes and destablizing changes get a little more bake time. @joshfree is looking into that.

Thanks for your continued patience, @terrajobst has just posted our API Review process proposal, feel free to comment over here: https://github.com/dotnet/corefx/issues/294.

The proposed LoadAsync methods in this PR take a CancellationToken as the last parameter. Should an overload be included that doesn't take CancellationToken?

For example:

public static Task<XDocument> LoadAsync(string uri, LoadOptions options)
{
    return LoadAsync(uri, options, CancellationToken.None);
}

public static async Task<XDocument> LoadAsync(string uri, LoadOptions options, CancellationToken token)
{
    ...
}

Also, most existing methods in the Framework that take a CancellationToken parameter name the parameter cancellationToken, not token. I personally don't have a strong opinion about this either way, but it might be best to be consistent with how it's predominantly named elsewhere.

re: "token" vs "cancellationToken", you're right, and I'd rather maintain consistency with existing conventions. I'll change that.

re: overloads that don't support cancellation, I didn't include them simply because I never use them (why implement an async API if you won't support cancellation?) and didn't think about it. I can add if wanted.

I didn't include them simply because I never use them (why implement an async API if you won't support cancellation?) and didn't think about it. I can add if wanted.

I'll let the official API reviewers weigh in, but this is mentioned in the Task-based Asynchronous Pattern (TAP) design guidelines:

In TAP, cancellation is optional for both asynchronous method implementers and asynchronous method consumers. If an operation allows cancellation, it exposes an overload of the asynchronous method that accepts a cancellation token (CancellationToken instance). By convention, the parameter is named cancellationToken.

We've reviewed these APIs.

We're OK with adding these APIs but in order to be complete, Save needs corresponding methods as well.

Can one of the admins approve this PR test or whitelist this user? ('@dotnet-bot add to whitelist' to whitelist, '@dotnet-bot okay to test' to accept for PR, '@dotnet-bot test this please' to retest. Case sensitive).

Thank you @terrajobst.

In addition to Save(), there is also XNode.ReadFrom() and XNode.WriteTo() which should get an async implementation. I will add these to the PR.

I excluded Save() initially due to there not really being a clean way to implement... will involve essentially duplicating all relevant implementation.

@scalablecory, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

Maybe I just missed them or discussion of them, but are there tests to accompany the new APIs?

@scalablecory We've changed our branching strategy in a way that impacts this PR.

When you're ready to continue work on this, you'll need to either recreate the PR against the 'future' branch or wait until after we have snapped the first release branch before we can merge it in to master.

See https://github.com/dotnet/corefx/wiki/branching-guide

Hrm, I wonder if I could rebase it to keep the same PR.

Unfortunately you can't change the target branch of an existing PR. You have to close this and open a new one targeted at the new branch. 😭

Add your 👍 here: isaacs/github#18 😉

@scalablecory , it is so simple! :just-kidding: :)
You can open a new PR against future branch. You may want to work in your corresponding future branch:

cd \my\projects\path\corefx
git remote add corefx https://github.com/dotnet/corefx
git fetch corefx
git checkout future
git merge --no-ff master
# resolve conflicts
# add more commits, for example, addressing review comments here
git push

Then future...scalablecory:future and open a new PR, where you reference the link to this PR in description to retain the comment history / all this interesting conversation. :)

@scalablecory, just checking in on this. Do you need any assistance in order to move forward?

@stephentoub Someone reorganized the master repo without using "git mv" and it wrecked my merge. I'm finding some time to redo the changes against future.

without using "git mv"

JFYI, Git doesn't store move/rename information. git mv identical to copy file to another location and remove it from old.
You can try this advice http://stackoverflow.com/a/4722641/61505 , maybe git will detect renames with this changes (check if you have line too many files skipping inexact rename detection in git output)

@stephentoub, since OP is having trouble with rebasing; would it make sense if this gets superseded by a separate PR, for instance, with an additional commit of yours fixing the merge conflicts?

It's up to Cory whether he'd like that or not.

@scalablecory, are you still pursuing this? Otherwise someone else can rebase these commits on your behalf and send a new PR.

@stephentoub If you've got the time to fix this patch, please do! I'm unfortunately too busy to follow through.

Ok, I'll see what I can do. Thanks.

Replaced by #2436.