These tests are just asserting current behavior, not that the current behavior is necessarily 100% correct (e.g. typing private)

this contains the API we'd like to expose to intellicode. Once it's all correct, I plan to add these to pythia external access. I don't like the names I came up with, so feel free to suggest others!

I believe this is essentially how we discussed the API, however I encountered a few issues when implementing.

1. If intellicode wants to ask us for multiple intents, this won't support it (or multiple intents with different intent data, e.g. generate ctor with different fields). This could be solved by calling the API multiple times though.
2. If we want to return multiple different kinds of actions for a single intent (for example, generate ctor can offer a ctor for all fields, or to delegate through an existing ctor).
3. Returning edits for more than just a single document (see comment below)

yes. i think this should be a one->many request, or many->many request. If we make it one->many, then they can call us multiple times. i think that's likely fine, unelss there is a deep perf issue here, or we want to know all the set of inputs to pick the best set of outputs.

we'll have to discuss with them about this.

This is a little fragile in that nothing is preventing this provider from changing the actions. One idea I had was to create something like a SolutionChangesCodeAction that enforces only ApplyChangesOperations can be returned. We'd have to do something similar for code actions with options (e.g. once given options, it only returns applychangesoperations). But I'm open to better ideas

As above, it seems like we need to be able to support multiple actions being returned from a single intent

yes. a good case to bring up with them.

We can show the first in the lightbuble and all in the completion list

this is an internal API used to provide calculate the sln for applying a specific intent

I remember we discussed having intellicode send us the edits to rewind plus the initial snapshot as well as them sending us the initial snapshot. But it doesn't seem like it matters who does the rewinding so I left it as input snapshot. Unless the issue is that they cannot get an actual textsnapshot, and we would have to rewind based on document

yeah, let's talk to them.

ok for now. likely somethign that should either bail gracefully on, or throw real ArgumentNullExceptions on. this is a public entrypoint, so Contracts are not right.

If we give something that is not correct, throw an exception like you do here. That will get our attention directly in the telemetry

annoyed that you have to do this. what's the consequence if we dont'? i feel like as we're not applying to the solution, we likely don't need to change this ourselves.

I don't think it applies to generate ctor specifically (I couldn't think of a scenario anyway), but for other code actions it definitely could

e..g renaming

class Class2
{
    int someField = 1;

    void M()
    {
        #if NETCOREAPP3_1
        someField++;
        #endif

        #if NETSTANDARD2_0
        someField--;
        #endif
    }
}

needs to account for changes in both documents

something is very smelly here. we should discuss this later.

my take on this is: the caller should specify if they even care about optiosn or not. for Intents, we woudl not, so we wouldnt' have to get them and unpack them.

In one of the next versions we can see if we can provide these based on opened files till the refactoring detection. Or if you can scan/see for the common usage in the solution. It is good to what was done in the result so we can report over the chooses we made.

i think this api shape is overall fine for now. i do think in teh future we should probably ask the platform/intellicode to provide a 'prior' ITextSnapshot.

We'll probably need to doc that it will not be attached to an actual text buffer.

nit: sort this above TextChanges.
nit: you use fields here, but properties in teh above object. can you be consistent for both?

since this will be a public API, properties are almost certainly more appropraite as they give more flexibility in teh future to change things. (we'd also likely use itnerfaces here in the future)

will move, but what are the sorting rules here? alphabetical, based on type?

interesting case. id this actually come up with your test cases?

It did not for this particular refactoring - generate ctor will only generate parameters for the current tfm for example. However, it will definitely come up later if we do any kind of rename. I could leave it out until then, but I don't think there's any harm in leaving it in - we always would want to return edits based on whatever is actually in the buffer (the merged result)

it's an interestnig pattern. will be worth seeing if we can pull it out to a common place (maybe a base class) in the future so that as we do more intents we can not repeat this everywhere.

so Someone in the stack should actually ensure that we do not offer changes that have effects beyond just editing the current document. it probably should be at a higher level so that not all intent providers have to do that determination.

In the intent processor I just drop everything that is not for the current document. Should I instead throw if we have changes outside of a single document? It seems like we will eventually support more than the single document though

that's cool. woudl love to see a demo where this works. i.e. where they pass in "we think tehy are generating teh constructor with props X and Y".

i generally hate this as it looks like it is popping up UI here :) is there like an extracted static helper we could call instead that just has the logic?

Hmm I think the CodeActionWithOptions contract is fairly clear - the options are only determined when GetOptions is called or getoperations is not given options. If the API takes in the options, the options have already been determined. But I will extract this out in a followup where I do some more cleanup on the implementation here (all the logic is in getoperationsasync right now)