Skip to content

WIP: Enable DateTime completion in string interpolation format strings.#43569

Merged
CyrusNajmabadi merged 2 commits intodotnet:masterfrom
zaytsev-victor:Fixes43267
Jun 23, 2020
Merged

WIP: Enable DateTime completion in string interpolation format strings.#43569
CyrusNajmabadi merged 2 commits intodotnet:masterfrom
zaytsev-victor:Fixes43267

Conversation

@zaytsev-victor
Copy link
Contributor

Fixes #43267

@zaytsev-victor zaytsev-victor requested a review from a team as a code owner April 22, 2020 20:33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what to do with this line. CompletionProvider receives position of '}' of format expression inside of string interpolation. I get previous token for analization, but SpanStart for the token is position - 1. I've temporarily commented this line. @CyrusNajmabadi Do you have any idea what the best way to handle it?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not really understanding the issue. stringTokenOpt should be the appropriate string token for either case, unless the caller isn't passing the right thing.

@zaytsev-victor zaytsev-victor changed the title Enable DateTime completion in string interpolation format strings. WIP: Enable DateTime completion in string interpolation format strings. Apr 22, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

why were these removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IsMethodArgument contains these conditions:

private static bool IsMethodArgument(SyntaxToken token, ISyntaxFacts syntaxFacts)
            => syntaxFacts.IsLiteralExpression(token.Parent) &&
               syntaxFacts.IsArgument(token.Parent!.Parent);

Copy link
Contributor

Choose a reason for hiding this comment

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

no equivalent change for VB?

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get InterpolatedStringTextToken. For ToString method it returns the same token.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird. If I was after the token in a format clause, what token is returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

root.FindToken(position) returns } for $"Text {date:$$}" and for $"Text {date:M$$}"

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. but what does it return for FindTokenOnLeftOfPosition? i would expect it to return the : token then. Is that not hte case? If FindToken returns the } chec if that's the case (i.e. you're at the start of that token, and move back one token. That seems like the simpler approach to take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FindTokenOnLeftOfPosition returns : for $"Text {date:$$}" and :M for $"Text {date:$$}". That exactly what we need here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. can you please doc in the code. thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

VB tests?

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 22, 2020
@CyrusNajmabadi
Copy link
Contributor

@zaytsev-victor What woudl you like to do with this PR? Thanks! :)

@zaytsev-victor
Copy link
Contributor Author

@CyrusNajmabadi I will be very busy until the end of this week but I can finish it on the next week.

I need to add comment for line var token = root.FindTokenOnLeftOfPosition(position);.

Could you please help me with this line?

This isn't work in string interpolation case. Should I delete it or rework somehow?

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Jun 4, 2020

Could you please help me with this line?

Once you make your changes, let me know. I'll debug through to see waht hte issue is.

@CyrusNajmabadi
Copy link
Contributor

Hey @zaytsev-victor will you be able to get to this? or would you want us to take this over for you? Thanks!

@CyrusNajmabadi
Copy link
Contributor

Hey @zaytsev-victor will you be able to get to this? or would you want us to take this over for you? Thanks!

@CyrusNajmabadi
Copy link
Contributor

I'm going to look at thsi to see what it woudl take to support this scenario.

@CyrusNajmabadi CyrusNajmabadi merged commit 4129ecb into dotnet:master Jun 23, 2020
@ghost ghost added this to the Next milestone Jun 23, 2020
@CyrusNajmabadi
Copy link
Contributor

@zaytsev-victor thanks for the PR!

@dibarbet dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DateTime completion should work in string interpolation format strings.

4 participants