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

Fixing off by 1 error for Negation To Substraction CodeFix #882

Merged
merged 3 commits into from
Jan 22, 2022

Conversation

jasiozet
Copy link
Contributor

@jasiozet jasiozet commented Jan 18, 2022

Fixing of by 1 error, using dec instead of inc.

Fixes ionide/ionide-vscode-fsharp#1657

@baronfel
Copy link
Contributor

baronfel commented Jan 18, 2022

Looks good! Could you try to add a test? The tests for most code fixes are here, and are very similar in structure. Here's a rough checklist:

  • create a script file in the test data folders similar to existing tests
  • make a new test suite (here's an example of one: https://github.com/fsharp/FsAutoComplete/blob/main/test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs#L411-L447)
  • point that test suite at your test script file in the server let binding
  • in your CodeActionParams, set the location to the code position that should trigger the codefix
  • assert that the return value from the codefix contains the change you want to see (in this case that "- " appears at the position you expect)
  • add your test suite to the list at the bottom of the file

@jasiozet
Copy link
Contributor Author

Of course, I'll try, but this will take me a little bit longer probably.
Any idea why only one of the builds failed?

@baronfel
Copy link
Contributor

The failure isn't your fault, some of the tests
on a certain component are flaky and so we take a ...probabilistic view on success.

@jasiozet
Copy link
Contributor Author

Ok I think I managed to fix it ok

@baronfel
Copy link
Contributor

Thanks for doing this @jasiozet, and congratulations on your first Pull Request ever on GitHub! I'm so happy that you chose to contribute :)

@baronfel baronfel changed the title Fixing of by 1 error for Negation To Substraction CodeFix Fixing off by 1 error for Negation To Substraction CodeFix Jan 22, 2022
@baronfel baronfel merged commit a7af704 into ionide:main Jan 22, 2022
@jasiozet
Copy link
Contributor Author

The credit goes all to you. There were 2 hard parts - setting up env, debugging on local machine & knowing were to look in the codebase - you helped me greatly with both.

Super happy to be able to do the smaller part on my own.

@baronfel
Copy link
Contributor

This was released in FSAC 0.50.0 just now, and I hope to update ionide itself over the next couple days.

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.

Suggestion for "Use substraction instead of negation" produces incorrect result
3 participants