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.

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

Add missing open declaration as possible issue when warning about uppercase pattern names #8936

Merged
merged 2 commits into from
Apr 27, 2020

Conversation

jbeeko
Copy link
Contributor

@jbeeko jbeeko commented Apr 12, 2020

The following code:

module A =
    type A = Foo | Bar
module B =
    open A
    let a = Foo
module C =
    open B
    match a with
    | Bar -> Bar.ToString()

Will result in the following compiler warning:

      | Bar -> Bar.ToString()
  ------^^^

stdin(9,7): warning FS0049: Uppercase variable identifiers should not generally be used in patterns, and may indicate a misspelt pattern name.

While a misspelling may be the culprit in the experience of our team it is also common for this to be due to a missing open declaration as in the example above. In the case where this is due to a missing open declaration the warning message does not help at all and has left team members quite confused.

This PR changes the warning resource string so that the the following warning is presented:

      | Bar -> "bar";;
  ------^^^

stdin(18,7): warning FS0049: Uppercase variable identifiers should not generally be used in patterns, and may indicate a missing open declaration or a misspelt pattern name.

Unfortunately I have no ability to contribute messages in the other languages supported.

See #6712

@dnfclas
Copy link

dnfclas commented Apr 12, 2020

CLA assistant check
All CLA requirements met.

@cartermp
Copy link
Contributor

@jbeeko Some failing tests:

Conformance\Expressions\DataExpressions\QueryExpressions (W_UppercaseIdentifier01.fs) -- failed
Conformance\Expressions\DataExpressions\QueryExpressions (W_UppercaseIdentifier02.fs) -- failed
Conformance\Expressions\DataExpressions\QueryExpressions (W_UppercaseIdentifier03.fs) -- failed

https://dev.azure.com/dnceng/public/_build/results?buildId=597987&view=ms.vss-test-web.build-test-results-tab&runId=18834804&resultId=100292&paneView=debug

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Apr 14, 2020

Unfortunately I have no ability to contribute messages in the other languages supported.

@jbeeko If I'm not mistaken, that's automatically done by the translation team if Microsoft, which is triggered upon changes in the resource files. I forgot the exact procedure, but I'm sure @cartermp can point you in the right direction.

Edit: sorry, @cartermp didn't see you were already in this thread.

@jbeeko
Copy link
Contributor Author

jbeeko commented Apr 18, 2020

@cartermp thanks, I'll see how I can replicate those failures locally.

@jbeeko
Copy link
Contributor Author

jbeeko commented Apr 19, 2020

I changed the warning to :

Uppercase variable identifiers should not generally be used in patterns, and may indicate a missing open declaration or a misspelt pattern name.

But it occurs to me it this is a bit prescriptive, it could also be:

Uppercase variable identifiers should not generally be used in patterns, and may indicate a a misspelt or out of scope pattern name.

On balance I prefer the advise to add an open statement because it provides concrete advice.

@cartermp
Copy link
Contributor

@jbeeko I think the first warning is pretty good.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Apr 24, 2020

Uppercase variable identifiers should not generally be used in patterns, and may indicate a missing open declaration or a misspelt pattern name.

Fwiw, I prefer the first one too (btw, the 2nd one has a double 'a a').

@KevinRansom
Copy link
Member

@jbeeko , thank you for taking care of this.

@KevinRansom KevinRansom merged commit 97a154c into dotnet:master Apr 27, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
…ercase pattern names (dotnet#8936)

* add  missingg open as poss reason

* change working in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants