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

Suggest default in switch statement #800

Closed
wants to merge 2 commits into from
Closed

Suggest default in switch statement #800

wants to merge 2 commits into from

Conversation

cezarant
Copy link

@cezarant cezarant commented Jun 6, 2016

Fixes #780
CC0120: Suggest default for switch statements (C#)

This is my first pull request, if there is a problem , like a feedback.
Thank you!

cezar teste added 2 commits May 29, 2016 21:51
- Load a new branch, neater without the results of running tests.
- include test for Boolean variables
- Include comma in DiagnosticId.cs
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 91.584% when pulling 9a1d9c1 on cezarant:suggestDefault into a657cab on code-cracker:master.

var diagnostics = from dia in context.SemanticModel.GetDiagnostics()
where dia.Id == "CS0151"
select dia;
if (diagnostics.ToList().Count == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would simplify this down to:

if (!diagnostics.Any())

@cezarant
Copy link
Author

cezarant commented Jun 7, 2016

Thanks for the feedback @thorgeirk11 !
I will make the changes indicated :)

@carloscds carloscds self-assigned this Jun 14, 2016
@carloscds
Copy link
Contributor

Hi @cezarant, this code not raise analyzer:

var s = false;
            switch (s)
            {
                case false:
                    {
                        break;
                    }
            }

@giggio
Copy link
Member

giggio commented Aug 7, 2016

There are several small problems, @cezarant, are you going to move forward with this PR?

@giggio
Copy link
Member

giggio commented Aug 7, 2016

@carloscds

Actually, this code:

var s = false;
            switch (s)
            {
                case false:
                    {
                        break;
                    }
            }

Should raise a diagnostic, as it does not have a case true. We can't go on trying to verify if s had an assignment, this would make this analyzer impossible to finish. We should keep the analysis to the switch statement, IMO.

@cezarant
Copy link
Author

cezarant commented Aug 7, 2016

@giggio I will study more about the Code Cracker. I did not think I'd find so many difficulties in carrying out this analyzer. Forgive me for doing pull Request with a low coverage tests. I will be more cautious next time.

@carloscds
Copy link
Contributor

@cezarant Thanks for your collaboration and go ahead!

@carloscds
Copy link
Contributor

@cezarant Do you have some news about this ?

@cezarant
Copy link
Author

Hi @carloscds I was waiting for feedback for Fix #808, but I can work here. Following the above suggestions. I think I have to make a rebasing of the code that was loaded. Right?

@carloscds
Copy link
Contributor

@cezarant We can process this PR separately.

@cezarant
Copy link
Author

Hi devs! Anybody here have had problems to build the project, when change of machine?
I receive this error (VSSDK1031) when i build.
I am making several attempts to resolve the error, but if someone has been there and can help me, I would appreciate

@carloscds
Copy link
Contributor

@cezarant Here project is compiling. Do you check for any extension installed on last days ?

@cezarant
Copy link
Author

Hello @carloscds I did a format in my notebook, and re-install the Visual Studio. It is the only abnormal thing I did. Now, i unmarked options on the VSIX tab, and seems to have settled.

vsix
If no IDE error happen more, I will upload the changes soon.

@carloscds
Copy link
Contributor

@cezarant CodeCracker users VSIX project.

@cezarant
Copy link
Author

@carloscds I had two problems with the original branch that gave rise to this PR:

  • The build generate the error VSSDK1031, and only works when the retreat option to "Create VSIX during the build"
  • The rebase is taking too long to be done

So, I clone the actual version of the code cracker, created a new branch and put the changes.
The new branch is able to merge, I wonder if you see any problem if I generate another pull request for this issue.
I did not want to do that. But it is giving a lot of work to restore the old branch.

@carloscds
Copy link
Contributor

@cezarant This branch has some conflicts, can you update it ?

@cezarant
Copy link
Author

@carloscds Being my first analyzer, the original branch of this pull request has unnecessary modifications to files that were not part of the issue. I created a new pull request, starting from the beginning and already contemplating the suggestions mentioned above.

It is a bad idea?

@carloscds
Copy link
Contributor

@cezarant Look:
image

@cezarant
Copy link
Author

Hello @carloscds i resolved the conflicts in the the branch that I used to create this pull request.
However, as this branch is quite old (06 Jun) , the comparison with the current code generated 217 modified files. Even though I made the rebasing.

I feel unsafe to perform the pull request that branch. I can make a new rebasing,
to reduce the number of modified files, but I think it would be better to close this pull request, and to continue on the PR #851.
Because in addition to having had an increase in test coverage, PR #851 had a much smaller number of modified files: only six.

I'm sorry to have created another pull request, but could not find another way to check test coverage.

@carloscds
Copy link
Contributor

@cezarant If you will create a new PR, please close this.

@cezarant cezarant closed this Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants