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

Use recommended postfix notation for generics #14530

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Use recommended postfix notation for generics #14530

merged 3 commits into from
Jan 5, 2023

Conversation

cmeeren
Copy link
Contributor

@cmeeren cmeeren commented Jan 2, 2023

Fixes #14465

Uses postfix notation for types listed in the F# code formatting guidelines:

  1. For F# Lists, use the postfix form: int list rather than list<int>.
  2. For F# Options, use the postfix form: int option rather than option<int>.
  3. For F# Value Options, use the postfix form: int voption rather than voption<int>.
  4. For F# arrays, use the syntactic name int[] rather than int array or array<int>.
  5. For Reference Cells, use int ref rather than ref<int> or Ref<int>.

@cmeeren cmeeren requested a review from a team as a code owner January 2, 2023 14:19
@edgarfgp
Copy link
Contributor

edgarfgp commented Jan 2, 2023

Fixes #14465

Uses postfix notation for types listed in the F# code formatting guidelines:

  1. For F# Lists, use the postfix form: int list rather than list<int>.
  2. For F# Options, use the postfix form: int option rather than option<int>.
  3. For F# Value Options, use the postfix form: int voption rather than voption<int>.
  4. For F# arrays, use the syntactic name int[] rather than int array or array<int>.
  5. For Reference Cells, use int ref rather than ref<int> or Ref<int>.

I think number 4. should be For F# arrays, use the syntactic name int array rather than int[] or array<int>. as #13700 is already in main. cc @vzarytovskii

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 2, 2023

Note that I simply quoted the whole list for completeness. This PR does not touch any array notation, as per my understanding of #14465 (comment).

@edgarfgp
Copy link
Contributor

edgarfgp commented Jan 2, 2023

Note that I simply quoted the whole list for completeness. This PR does not touch any array notation, as per my understanding of #14465 (comment).

Thanks for clarifying :)

dsyme
dsyme previously approved these changes Jan 3, 2023
Copy link
Member

@psfinaki psfinaki 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, @cmeeren, it's important to be consistent at least with our own guidelines.
I'm afraid we need to update the correspondent .fs files, that's why we have all those CI errors.

I am kind of on the fence if this should be an error or not actually. In theory, this is compilable so the error is a bit harsh, but practically I can't see any reason or benefit for such an inconsistency between .fs and .fsi files and it's easy to address both.

Probably should be a warning though.

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 4, 2023

Ok, do you want me to go ahead and perform the same replacement in the .fs files corresponding to the .fsi files I changed?

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 4, 2023

And importantly, do you want me to update only the required places (corresponding to the .fsi updates), or all occurrences in the relevant files?

@psfinaki
Copy link
Member

psfinaki commented Jan 4, 2023

@cmeeren well yes it would be awesome if you fix the required places, then the CI will pass and we could already merge the PR. But if you have time, feel free to fix some other occurrences as well - you will definitely get extra F# karma points for that :)

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 4, 2023

I'll do that!

For the record, in case the effect of the signatures aren't clear: The primary reason I am making this change isn't because I am heavily invested in minute stylistic details in FSharp.Core, but because the non-idiomatic notations propagate (via type inference and Intellisense/LineLens etc.) into user code bases.

@psfinaki
Copy link
Member

psfinaki commented Jan 4, 2023

Thanks a lot! Well, that's anyway beneficial for the F# repo itself as well :)

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 4, 2023

Hopefully works now. Cleaned up the commits. I have removed all edits from test files, since I am unsure what is safe to change here (some of the tests may be testing the different notations for all I know).

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 4, 2023

Note that for a while I was thoroughly confused due to very many usages of List<_> actually being the System.Collections.Generic one. You could consider changing that to the ResizeArray alias everywhere for clarity. I can't prioritize making a PR for that at the moment. Still, I wanted to mention it.

@psfinaki
Copy link
Member

psfinaki commented Jan 5, 2023

@cmeeren thanks for the update, CI is green, @dsyme please reapprove.

Note that for a while I was thoroughly confused due to very many usages of List<_> actually being the System.Collections.Generic one. You could consider changing that to the ResizeArray alias everywhere for clarity.

I am on the fence with this actually, but feel free to create a ticket and elaborate a bit, someone can pick it up then.

Note I created a ticket for an analyzer to force the conventions, I think that would be useful and expected by C# users.

@dsyme
Copy link
Contributor

dsyme commented Jan 5, 2023

You could consider changing that to the ResizeArray alias everywhere for clarity.

I agree we should do this in our own code

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

Successfully merging this pull request may close these issues.

Use idiomatic/canonical option type annotations in Result module
5 participants