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 a compiler warning for lowercase literals in patterns #666

Closed
wants to merge 4 commits into from
Closed

Add a compiler warning for lowercase literals in patterns #666

wants to merge 4 commits into from

Conversation

dungpa
Copy link
Contributor

@dungpa dungpa commented Oct 3, 2015

UserVoice request (approved for F# 4.x): http://fslang.uservoice.com/forums/245727-f-language/suggestions/6668206-warn-when-literal-attribute-is-used-with-lowercase

This is a small and low-risk change to warn users when lowercase literals are ignored in favor of variable binding patterns.

Unit tests have been added.

@neoeinstein
Copy link
Contributor

I like it, and it looks good from my review.

@dungpa
Copy link
Contributor Author

dungpa commented Oct 5, 2015

@neoeinstein Thanks for reviewing. It's really useful.

@vasily-kirichenko
Copy link
Contributor

Very important warning. Great job! 👍

| Some x -> x
| Some value ->
let name = id.idText
if not (String.IsNullOrEmpty name) && Char.IsLower(name.[0]) then
Copy link

Choose a reason for hiding this comment

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

when would name be empty?

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 it will happen. The check is there to make sure that the index access (name.[0]) is totally safe.

Copy link

Choose a reason for hiding this comment

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

Ok me neither. However it does seem surprising that a identifier is the empty string.

@dsyme
Copy link
Contributor

dsyme commented Oct 8, 2015

This looks good to me.

@KevinRansom
Copy link
Member

committed.

@KevinRansom KevinRansom closed this Oct 9, 2015
@dungpa dungpa deleted the literals branch October 9, 2015 07:22
@dungpa
Copy link
Contributor Author

dungpa commented Oct 9, 2015

@KevinRansom I think it's better to reference the PR number and the issue number (if there is one) in the commit message. As it is committed now, it's really hard to blame the original author for bugs in the original PR :-).

@KevinRansom
Copy link
Member

I screwed up the commit, I just reverted and am going to attribute it properly.
Sorry, it's kind a late here.

@PatrickMcDonald
Copy link
Contributor

A quick look at the PR # explains everything!

KevinRansom pushed a commit that referenced this pull request Oct 9, 2015
@KevinRansom
Copy link
Member

When I rebased, I forgot to set the author correctly. There are many times when I wished we just pressed the merge button.

Once again sorry for the brain-fart. Now it's time for bed.

Kevin

@forki
Copy link
Contributor

forki commented Oct 9, 2015

Dude, you are the one that is merging. Just make that decision to use the merge button. ;-)

@dungpa
Copy link
Contributor Author

dungpa commented Oct 9, 2015

@KevinRansom No worries. I don't mind at all. The main problem is traceability and maintainability that we have to be careful when merging PRs.

@vasily-kirichenko
Copy link
Contributor

@forki 👍

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.

9 participants