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

Allow elmish style tuples #10186

Merged
merged 4 commits into from
Jan 29, 2019
Merged

Allow elmish style tuples #10186

merged 4 commits into from
Jan 29, 2019

Conversation

forki
Copy link
Contributor

@forki forki commented Jan 28, 2019

Since Fsharplint is using this styleguide as source and ionide is using fsharplint, it seems this guide is going to be the canonical styleguide. In general I think this is a good thing and I try to be not too opionated here (which is hard).
Anyway, I'd like to suggest a small change to the very restrictive tuple instantiation. The new fsharplint is complaining on thousands of lines in our elmish update functions. Stuff that we as community suggested to use for everyone in safe stack. Basically all samples and all existing safe stack code is now considered as bad.
I'm not sure if my wording here is good, but I want to open up the discussion.

Since Fsharplint is using this styleguide as source and ionide is using fsharplint, it seems this guide is going to be the canonical styleguide. In general I think this is a good thing and I try to be not too opionated here (which is hard).
Anyway, I'd like to suggest a small change to the very restrictive tuple instantiation. The new fsharplint is complaining on thousands of lines in our elmish update functions. Stuff that we as community suggested to use for everyone in safe stack. Basically all samples and all existing safe stack code is now considered as bad.
I'm not sure if my wording here is good, but I want to open up the discussion.
@smoothdeveloper
Copy link
Contributor

A few times, I've been facing ambiguous issues around tuple instanciation and pattern matching, and those issues have been solved using explicit parens (dotnet/fsharp#5259 is example of me tripping on this), like it is the case in several areas of F#.

  • Do I like to wrap all the expressions disregarding the fact there is no ambiguity: no
  • Is it a better default to have tuple always wrapped: probably if we don't have better solution
  • Is it better to have the warning show only for tuples that are ambiguous: yes

I feel code is more readable when parens are only added when necessary or when it clarifies possible ambiguity (despite not being necessary in all cases, the guideline to wrap would apply when some of the elements are not mere identifers or literals, but more complex expressions).

I'd like to relax the guidelines along those lines.

Ideally, in the tradition of Resharper in C#, I'd like the tooling to propose me to wrap / unwrap expressions which has extraneous parens or lacking parens, and have some place to define my prefered styling.

@MangelMaxime
Copy link
Contributor

I think the addition requested by @forki make sense when we consider Elmish applications.

@forki
Copy link
Contributor Author

forki commented Jan 28, 2019

There is another argument for lifting the restriction: symmetry. If we don't require it in destructering/pattern matching, then we should not do it in construction.

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.

Some quick changes, but I otherwise think that this is good.

Can you also update line 258 along the following lines?

"A tuple instantiation should generally have the right-hand side parenthesized, and the [...]"

docs/fsharp/style-guide/formatting.md Outdated Show resolved Hide resolved
docs/fsharp/style-guide/formatting.md Outdated Show resolved Hide resolved
docs/fsharp/style-guide/formatting.md Outdated Show resolved Hide resolved
@forki
Copy link
Contributor Author

forki commented Jan 29, 2019

Applied your suggestions. Thx. (btw that new github feature is rad)

@cartermp
Copy link
Contributor

@forki Yep, whoever built that out deserves a raise

@cartermp
Copy link
Contributor

Here's what your change looks like:
image

Merging, thanks @forki

@cartermp cartermp merged commit 272367c into dotnet:master Jan 29, 2019
@mairaw
Copy link
Contributor

mairaw commented Jan 29, 2019

You can also batch them in one commit via the Files tab. I love that feature!

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

Successfully merging this pull request may close these issues.

6 participants