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 idiomatic/canonical option type annotations in Result module #14465

Closed
cmeeren opened this issue Dec 12, 2022 · 7 comments · Fixed by #14530
Closed

Use idiomatic/canonical option type annotations in Result module #14465

cmeeren opened this issue Dec 12, 2022 · 7 comments · Fixed by #14530

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Dec 12, 2022

Result.toOption has return type Option<'T>. The idiomatic/canonical notation is 'T option. The use of Option<'T> here means that Option<'T> shows up in code/line lens and tooltips all over my codebase when using this function, whereas I would prefer 'T option.

I haven't checked, but I am guessing that this could apply to other functions in the Result module, too.

For completeness, note that the same kind of issue would also apply to functions typed with List<'T> instead of 'T list, and other similar types. (I haven't checked or encountered any, though.)

@1eyewonder
Copy link

Are we asking to just update the signature file for result to show something like this or am I misunderstanding the issue?
image

@cmeeren
Copy link
Contributor Author

cmeeren commented Dec 22, 2022

I don't know much about signature files vs. implementation files, but that's the transformations I am thinking of, yes. Except you should use voption instead of ValueOption, if I recall correctly.

@cmeeren
Copy link
Contributor Author

cmeeren commented Dec 22, 2022

These replacements could be done in all signature files, by the way, not just those pertaining to the Result module. (I don't know of any, but search/replace should make this easy.)

If you agree, I can make a PR for this. A couple of questions:

  • Apart from option, voption, and list, which other types have alternative canonical representations? What about 'a array, which is also 'a []? The code base should be consistent. I think I recall having seen @dsyme mention array notation in particular someplace, but I have no idea where or even when.
  • Is it OK with replacing one type per commit across all signature files?

@T-Gro
Copy link
Member

T-Gro commented Dec 28, 2022

I would recommend to limit a PR to a handful of "find/replace" operations and explain those in the PR.
That way, even if the diff is big, it will be easy to review.

Each replacement being a separate commit (with description telling which action caused it) sounds great.

Array usage has been recently unified (@edgarfgp did a lot of work there), I am not sure if there are any things left.

@edgarfgp
Copy link
Contributor

Regarding array changes. The remaining work is tracked here #14329.

  • Update FSharp.Core API reference
  • Update docs

@cmeeren
Copy link
Contributor Author

cmeeren commented Dec 28, 2022

So I would only be changing to postfix lowercase notation in signature files for these?

  • option
  • voption
  • list
  • ref

Apart from [], these are the only ones listed in the formatting guidelines, so I guess the list is complete.

@cmeeren
Copy link
Contributor Author

cmeeren commented Dec 30, 2022

Protip - GH doesn't notify about reactions, so only due diligence ensured I am now aware you agree and that I can proceed 😉

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 a pull request may close this issue.

5 participants