DotNetAnalyzers / NullParameterCheckRefactoring

Null Parameter Check Refactoring for Reference Type Parameters
MIT License
14 stars 4 forks source link

Add VB.net support #8

Closed AdamSpeight2008 closed 10 years ago

AdamSpeight2008 commented 10 years ago

Restructured Folder Layout. Added VB.net support (VS2015) Created merged VSIX

tugberkugurlu commented 10 years ago

folder layout is completely wrong. those should be under src.

tugberkugurlu commented 10 years ago

Do we really need a separate project for this? Cannot this be written in C# and target VB?

AdamSpeight2008 commented 10 years ago

@tugberkugurlu

There's nothing wrong with VB.net

My other diagnostic SFD the C# target is written in VB.net because VB.net has a better implementation of the switch statement.

tugberkugurlu commented 10 years ago

@AdamSpeight2008 I didn't say there is something wrong with VB.NET :smile: I don't think it's necessary here. You can implement this inside the same project with possibly a huge amount of code reuse (I'm just assuming).

AdamSpeight2008 commented 10 years ago

The syntax nodes of C# and VB.net are just different enough to be a pain the posterior.

AdamSpeight2008 commented 10 years ago

CSharp SyntaxNode != VB SyntaxNode They completely different entities. They only looks similar do to imports,

tugberkugurlu commented 10 years ago

@sharwell what do you think? Should this be written in VB?

AdamSpeight2008 commented 10 years ago

@tugberkugurlu I've only moved your refactoring to a CSharp folder, with the solution. (It is still in C#) I did this so potential the project names could be the same without a clash.

sharwell commented 10 years ago

It's very difficult to reuse code that operates on the syntax tree as opposed to strictly working with the semantic model. I wouldn't have a problem with implementing them in separate projects, but I can say it would slow me down a bit if someone filed a bug report against the VB implementation.

sharwell commented 10 years ago

I will propose a new project structure though, which more closely aligns with the original one created by @tugberkugurlu.

AdamSpeight2008 commented 10 years ago

See nullparametercheckrefactoring

nullparametercheckrefactoring2

sharwell commented 10 years ago

I'm almost done :)

sharwell commented 10 years ago

OK I just created pull request #10. It essentially "moves the C# code out of the way" so you can add a new project NullParameterCheckRefactoring.VB to match it. There is no need to create more than one VSIX project, since it's easy to include both assemblies in one package.

AdamSpeight2008 commented 10 years ago

The separate VSIX let you debug that specific analyser. The combine VSIX pulls from the two projects as MEF Component assets.

sharwell commented 10 years ago

I've never had a problem developing or debugging multiple assemblies within a single VSIX (some of my extensions are very complicated, but none of them involve both VB and C# assemblies together). Are there particular concerns you have regarding the functionality? While I certainly prefer a simpler project layout where possible, I don't want to force the use of a single VSIX if it will cause problems with the development cycle.

AdamSpeight2008 commented 10 years ago

@sharwell There are just .net assemblies. It doesn't matter what .net language they are written in both C# and VB.net compile to .net assemblies. The DLL I'm referencing could be written Nemerle, Boo, C#, VB

AdamSpeight2008 commented 10 years ago

Projects Names are now

AdamSpeight2008 commented 10 years ago

nullparametercheckrefactoring3

sharwell commented 10 years ago

@AdamSpeight2008 I implemented what I was thinking of in the following branch. I made sure to rebase your work so you still get credit for the VB implementation in the commit.

https://github.com/sharwell/NullParameterCheckRefactoring/compare/vb-support

How does that look to you?

tugberkugurlu commented 10 years ago

@sharwell can you put this project under src folder as well? https://github.com/sharwell/NullParameterCheckRefactoring/tree/c09a306130fd3af9052883efac1fe8308909f4ac/NullParameterCheckRefactoring

AdamSpeight2008 commented 10 years ago

I didn't know you where allowed to have .CSharp and .VB in Project names. That's why I tend to use (CS) and (VB).

AdamSpeight2008 commented 10 years ago

Also we need to add a release notes and getting started guide, I've used https://github.com/DotNetAnalyzers/NullParameterCheckRefactoring for the More Info Url

sharwell commented 10 years ago

@tugberkugurlu Those aren't referenced. I just forgot to remove them (which has now been corrected).

tugberkugurlu commented 10 years ago

@sharwell so, which one are we going to merge now :smile:?

AdamSpeight2008 commented 10 years ago

@tugberkugurlu I could alter the existing C# to include the update?

tugberkugurlu commented 10 years ago

@AdamSpeight2008 doesn't really matter to me as long as the structure is stays correct but it sounds like @sharwell did all the work in his fork?

AdamSpeight2008 commented 10 years ago

What is the structure?

sharwell commented 10 years ago

I don't have permission to push to @AdamSpeight2008 's repository, or to change the reference branch for this pull request. If you want to use my code (which includes @AdamSpeight2008 's work as one commit), then we need to do one of the following:

  1. @AdamSpeight2008 can reset his master branch to match my vb-support branch, and force push to his fork, thus automatically updating this pull request.
  2. I can submit a new pull request, which we merge instead of this one.
AdamSpeight2008 commented 10 years ago

How do I do the first?

sharwell commented 10 years ago

@AdamSpeight2008 I think it doesn't matter since @tugberkugurlu rebases changes anyway. Did you get a chance to try it out? If not, the steps to try it are:

  1. Add my fork as an additional remote for your working copy and run a fetch command to pull down my work.
  2. Check out my vb-support branch on your machine to test it out.
AdamSpeight2008 commented 10 years ago

I've just added you as a collaborator

AdamSpeight2008 commented 10 years ago

I've renamed my local copy and clone yours, and I look at the VB Branch

sharwell commented 10 years ago

I have to run some errands, but I look forward to seeing your feedback when I return. Supporting both languages is a huge step forward for this analyzer.

AdamSpeight2008 commented 10 years ago

I deleted by repo and the forked @sharwell