Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pars.fsy: capture empty interpolated string fills and report error #9849

Merged
merged 2 commits into from
Aug 15, 2020
Merged

pars.fsy: capture empty interpolated string fills and report error #9849

merged 2 commits into from
Aug 15, 2020

Conversation

yatli
Copy link

@yatli yatli commented Jul 31, 2020

No description provided.

@yatli yatli mentioned this pull request Jul 31, 2020
Merged
29 tasks
@dsyme
Copy link
Contributor

dsyme commented Jul 31, 2020

Looks good! Thank you!

@yatli yatli force-pushed the feature/string-interp branch from 01c5e03 to a2853e6 Compare July 31, 2020 12:06
@cartermp cartermp closed this Aug 5, 2020
@cartermp cartermp reopened this Aug 5, 2020
@cartermp
Copy link
Contributor

cartermp commented Aug 5, 2020

Somehow CI is failing on all legs here due to a typeload exception that appears unrelated to this PR?

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Aug 7, 2020 •
edited
Loading

In #9893 I made the suggestion to allow {} inside an interpolated string, wouldn't that be must easier, and besides, more useful? An empty expression can then just be removed, which will have the same effect as {null} or {None}, which only seems logical to me and works well with code-generators that don't have to consider the case special when the expression is empty.

@dsyme, @cartermp, what do you think? (also note that this case is not mentioned in the RFC, so technically this discussion is still open for debate 😄)

@cartermp cartermp changed the base branch from feature/string-interp to master August 7, 2020 23:16
@cartermp
Copy link
Contributor

cartermp commented Aug 7, 2020

@yatli @dsyme some conflicts in this one now. Want to handle them?

Yatao Li added 2 commits August 9, 2020 12:41
pars.fsy: capture empty interpolated string fills and report error
2866f43 
reduce empty interp fill error range, update tests

update error msg 3382
StringInterp: update test error code
0b15df5
@yatli yatli force-pushed the feature/string-interp branch from 126d6c0 to 0b15df5 Compare August 9, 2020 04:43
@yatli
Copy link
Author

yatli commented Aug 9, 2020

@cartermp rebased and force pushed.

@abelbraaksma
Copy link
Contributor

@charlesroddie convinced me why my proposal above is a bad idea, mainly because we have no precedence for making empty expressions legal, please disregard my suggestion.

@cartermp cartermp closed this Aug 9, 2020
@cartermp cartermp reopened this Aug 9, 2020
cartermp
cartermp approved these changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@cartermp
Copy link
Contributor

Merging as per @dsyme's feedback

@cartermp cartermp merged commit ee30fb0 into dotnet:master Aug 15, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
@nosami
pars.fsy: capture empty interpolated string fills and report error (d…
9129353 
…otnet#9849)

* pars.fsy: capture empty interpolated string fills and report error

reduce empty interp fill error range, update tests

update error msg 3382

* StringInterp: update test error code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsyme dsyme dsyme left review comments

@forki forki forki left review comments

@cartermp cartermp cartermp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yatli @dsyme @cartermp @abelbraaksma @forki