Skip to content

Updated FS0874. fixes #3642 #9813

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
Merged
cartermp merged 1 commit into from
Jul 28, 2020
Merged

Updated FS0874. fixes #3642 #9813

cartermp merged 1 commit into from
Jul 28, 2020

Conversation

@Lanayx
Copy link
Contributor

@Lanayx Lanayx commented Jul 28, 2020

No description provided.

@dnfadmin
Copy link

dnfadmin commented Jul 28, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

auduchinok
auduchinok reviewed Jul 28, 2020
View reviewed changes
src/fsharp/FSComp.txt Outdated
Copy link
Member

@auduchinok auduchinok Jul 28, 2020

Choose a reason for hiding this comment

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

Could we try using a bit simpler English? I can imagine how some non-native speakers could be confused by "neither/nor" here, actually making the error text more difficult to grasp. 🙂
Maybe something like "Mutable 'let' bindings can't be recursive or defined in recursive modules"?

Copy link
Contributor

@abelbraaksma abelbraaksma Jul 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should probably be "recursive modules or namespaces". A namespace can be rec too (unless that doesn't raise this error).

Copy link
Contributor

@abelbraaksma abelbraaksma Jul 28, 2020

Choose a reason for hiding this comment

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

Should note that @cartermp agreed earlier on this text proposal: #3642 (comment)

Copy link
Contributor

@cartermp cartermp Jul 28, 2020

Choose a reason for hiding this comment

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

I'd like the adjusted text. 'tis what reviews are good for :)

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 28, 2020 •
edited
Loading

@Lanayx, a suggestion: generally speaking, you shouldn't use the issue number in the commit message (which links the commit, not the PR), it's better to link the issue from the PR description. If you add Fixes #3642 at the top, it will also automatically close the original issue report once merged.

KevinRansom
KevinRansom approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Thanks for this

Kevin

cartermp
cartermp approved these changes Jul 28, 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.

Thanks!

@cartermp cartermp merged commit 8267c59 into dotnet:master Jul 28, 2020
@cartermp cartermp mentioned this pull request Jul 28, 2020
Closed
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
@Lanayx @nosami
Fix for FS0874 error text (dotnet#9813)
1558b36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@auduchinok auduchinok auduchinok left review comments

@abelbraaksma abelbraaksma abelbraaksma left review comments

@KevinRansom KevinRansom KevinRansom approved these changes

@cartermp cartermp cartermp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Lanayx @dnfadmin @abelbraaksma @auduchinok @KevinRansom @cartermp