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.

Sign up for GitHub

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

Jump to bottom

Option.get analyzer #31

Closed
sheridanchris opened this issue Nov 15, 2023 · 7 comments · Fixed by #32
Closed

Option.get analyzer #31

sheridanchris opened this issue Nov 15, 2023 · 7 comments · Fixed by #32
Labels
approved

Comments

@sheridanchris
Copy link
Contributor

sheridanchris commented Nov 15, 2023 •
edited
Loading

I think an analyzer for Option.get that promotes the graceful handling of the None case by using functions like Option.defaultValue or pattern matching should be added. I'd like to create a PR for this if you believe this is a useful addition.
@nojaf nojaf added the approved label Nov 15, 2023
@nojaf
Copy link
Contributor

nojaf commented Nov 15, 2023

Hi Chris,

Thank you for this suggestion! We like it and would be happy to receive a PR for this 🥳!
Don't hesitate to ask any technical questions if you have any.

@dawedawe
Copy link
Contributor

This sample could be of help for you to get started:
https://github.com/ionide/FSharp.Analyzers.SDK/blob/master/samples/OptionAnalyzer/Library.fs

@sheridanchris sheridanchris mentioned this issue Nov 15, 2023
Merged
@nojaf nojaf closed this as completed in #32 Nov 15, 2023
@Numpsy
Copy link
Contributor

Numpsy commented Nov 15, 2023

could / should there be something similar for ValueOption ?

@nojaf
Copy link
Contributor

nojaf commented Nov 15, 2023

Yeah, seems reasonable. Are you interested in submitting a PR?

@nojaf
Copy link
Contributor

nojaf commented Nov 15, 2023

Would probably be nothing more than adding an OR in if mfv.FullName = "Microsoft.FSharp.Core.Option.get" && args.Length = 1 then

@Numpsy
Copy link
Contributor

Numpsy commented Nov 15, 2023

I can have a go later if no one else gets to it first

@dawedawe
Copy link
Contributor

I think we should have the Option.Value analyzer from the SDK sample as well.

@sheridanchris sheridanchris mentioned this issue Nov 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add an analyzer that detects Option.get usage sheridanchris/ionide-analyzers
4 participants
@Numpsy @nojaf @dawedawe @sheridanchris