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

C# -> VB code comments are not converted #8

Closed
paul1956 opened this issue Jan 2, 2018 · 21 comments · Fixed by #512
Closed

C# -> VB code comments are not converted #8

paul1956 opened this issue Jan 2, 2018 · 21 comments · Fixed by #512
Assignees
Labels
C# -> VB Specific to C# -> VB conversion enhancement

Comments

@paul1956
Copy link

paul1956 commented Jan 2, 2018

It looks like they are just ignored.

@christophwille
Copy link
Member

christophwille commented Jan 2, 2018

Oops, I didn't copy over https://github.com/icsharpcode/RefactoringEssentials/blob/5857519150dc5084286e05f556a260558d700941/converter.txt - it has the latest recent list of limitations in the converter. And yes, comments are being ignored (main reason was that other statements are way more important to convert than comments)

@paul1956
Copy link
Author

paul1956 commented Jan 2, 2018

Any hints on where to start on implementing comments in C# to VB? I tried simply adding a .WithTriviaFrom and passing the original C# Node and that does nothing useful. Also getting a better match on line breaks, right now most multiline C# statements get converted to a single line. I have implemented coloring of source and output in a desktop converter and it was fairly trivial and have converted entire source to VB (passing all tests) if anyone is interested. One small suggestion, you might want to use LanguageNames throughout instead of "Visual Basic" and "C#" it makes the code less susceptible to typos.

@siegfriedpammer
Copy link
Member

siegfriedpammer commented Jan 3, 2018

When initially implementing the converter I could not come up with a way to deal with all the edge cases that arise from allowing preprocessor directives and comments.

For example:

#if SOMETHING
if (
#else
switch (
#endif
)

I guess we'd have to change the converter to convert each token individually. Not sure how to go about that.

@paul1956
Copy link
Author

paul1956 commented Jan 4, 2018

I don't see why your example is a problem, I just translate the trivia syntax but I have not done #If yet because I am having trouble getting into the DirectiveTrivia.

I added the 2 lines below to the top of every NodeVisitor function

Dim LeadingTriviaList As SyntaxTriviaList = CreateVBTriviaListFromCSharpNode(node.GetLeadingTrivia())
Dim TrailingTriviaList As SyntaxTriviaList = CreateVBTriviaListFromCSharpNode(node.GetTrailingTrivia())

and to the end of every return statement I add

.WithLeadingTrivia(LeadingTriviaList).WithTrailingTrivia(TrailingTriviaList)

I also had to do a little fixing to TidyImports and now most stuff just works. So far I have only partially implemented CreateVBTriviaListFromCSharpNode to handle regions and single line comments. The whole thing took about 4 hours over several days. I still need to see why I am missing a blank line between comments and the next statement and why some tests fail because of an extra blank line at the beginning that I am adding. Without the comments and region addition my VB version of your code passes 100% of the tests. I also have a desktop code converter with syntax highlighting that today converts files but I am adding folders and projects shortly. Eventually I would like to add As Clauses instead of Option Infer (which should be added to the top of every current conversion that uses var) but I am having lots of issues trying to get that to work even in a Visual Studio VSIX.

@paul1956
Copy link
Author

paul1956 commented Jan 4, 2018

The issue with the extra blank line is not mine, it is in your tests. You have an extra blank before class and I now preserve all the formatting.

	TestConversionCSharpToVisualBasic(@"
class TestClass 

But in the result you don't have the leading blank line

}", @"Class TestClass

@siegfriedpammer
Copy link
Member

You are welcome to submit a pull request with your changes. Please note that I have stepped down from my role as maintainer of the code converter long ago.

The project is looking for a new maintainer. However, if you submit a pull request I will take a look and merge it.

The issue with the extra blank line is not mine, it is in your tests.

I don't think you could call this an "issue". Since you are adding new information to the output that has not been there previously, it's obvious that the tests will have to be adjusted.

@paul1956
Copy link
Author

paul1956 commented Jan 4, 2018

I don't understand enough about GitHub to do a pull request. Agree an issue was a poor choice of words.

@siegfriedpammer
Copy link
Member

Take a look at https://yangsu.github.io/pull-request-tutorial/ for a simple step-by-step guide to pull requests. Thank you in advance for your work!

@siegfriedpammer
Copy link
Member

I've noticed that you had some VB.NET code in your comment above... why is your snippet showing VB.NET when our codebase is C#?

@paul1956
Copy link
Author

Because I code in VB, I have ported your entire codebase to VB (using a combination of this converter and others). If I decide to contribute (and can get it to work using instructions above) I with back port to C# before posting. I have Comment's and Regions working now. and am just working on additional tests. I am having issues preserving original formatting (notice the missing line feed at the top, the conversion is correct until the call to .NormalizeWhitespace at the end).

I would like to start with something very simple like using LanguageName instead of literals throughout the code. If that works I will tackle comments and regions followed by convert folder if there is interest.
image

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Jan 13, 2018

I've got a WIP branch for porting code comments VB -> C#, but it needs a bit of tidying up and some dedicated tests adding: https://github.com/GrahamTheCoder/CodeConverter/pull/4

I've tried to do it in a way that stays separate from the rest of the code, though there are edge cases around constructs that change shape which make this tricky, and might need a few overrides for certain types (hence using the visitor).
C# -> VB can probably be implemented as approximately the opposite conversion, happy to discuss further. Apologies for not noticing this thread sooner (I'd forgotten to watch the new repo) - I've now opened issues linking to my ongoing work so it's more visible in future.

@GrahamTheCoder GrahamTheCoder changed the title Is there a reason that comments are not converted? Code comments are not converted Jan 20, 2018
@GrahamTheCoder GrahamTheCoder changed the title Code comments are not converted C# -> VB code comments are not converted Feb 10, 2018
@GrahamTheCoder GrahamTheCoder self-assigned this Mar 6, 2018
@GrahamTheCoder GrahamTheCoder added the C# -> VB Specific to C# -> VB conversion label Apr 9, 2018
@GrahamTheCoder GrahamTheCoder removed their assignment May 2, 2018
@GrahamTheCoder
Copy link
Member

VB is under development again. Here's a recent change to where comments are allowed:
https://docs.microsoft.com/en-us/dotnet/visual-basic/getting-started/whats-new#visual-basic-160

@paul1956
Copy link
Author

paul1956 commented Nov 16, 2019

If you are looking for a more complete, open source C# to VB converter look at https://github.com/paul1956/CSharpToVB it tries to preserve all C# comments using the new feature. It also converts the true side of #If statements and many features of C# 8. As a test, some of the tests converts all C# code in Roslyn Repo without error. Some things I can convert are commented out and some things are converted with enough changes that I comment the VB code and leave the C# code as a comment.

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Nov 16, 2019

Thanks Paul, sounds good, I'll take a look. From what I understand, your repo is a combination of this project (converted to VB) and your own work. You previously mentioned you weren't sure how to pull request it. If I create a PR based on some or or all of your code, are you happy for that code to be merged back into this repo (under this repo's the current license terms)?

@paul1956
Copy link
Author

Absolutely I only want people to use it.

GrahamTheCoder added a commit that referenced this issue Feb 3, 2020
From commit: paul1956/CSharpToVB@fb33d1e
Used this commit of the converter
Permission explicitly given here: #8 (comment)
@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Feb 3, 2020

Hi @paul1956. I've just added in your implementation of converting trivia in PR #517 Thanks!
You might want to duplicate this (unverified) bugfix in your code: 094faf8
and look at 782ef61

@paul1956
Copy link
Author

paul1956 commented Feb 4, 2020

@GrahamTheCoder thanks for the catch. I made a lot of changes this week to trivia handling. It is very hard to correctly translate #Pragma Disable and Enable and it turns out to be useless anyway because the values all have to change. So now I translate them and then turn them into into pure comments using _ '. Also I have make a lot of changes to Tuple handling and added much better handling for className ClassName, there were many cases where I mistranslated it now becomes _className as ClassName. I have also removed all the errors I was ignoring for places where I couldn't produce valid code I always produce code that compiles without errors. Its been a busy week.

@GrahamTheCoder
Copy link
Member

I have the version from current master - I already removed the "Stop" statements.
I couldn't immediately find any tests, but if there are some, let me know and I'd love to move those across too.

If there are other areas you've made improvements in C#->VB, a description or link to the code/testcase would be really helpful.

@paul1956
Copy link
Author

paul1956 commented Feb 4, 2020

There is a test project, it does 2 things there are about 120 test that test sensitive fragile code and 3 tests that use Roslyn source and attempt to port all of it to VB and compiler each file the code will not run because I don't port the projects and comment out unsafe and some other things I can't port.

@GrahamTheCoder
Copy link
Member

Great I'll take a closer look. I think I just searched the solution for some particular bits of syntax and didn't find any

@paul1956
Copy link
Author

paul1956 commented Feb 4, 2020

https://github.com/paul1956/CSharpToVB/tree/master/CSharpToVB.Tests I get 88% code coverage almost all of it from translating Roslyn. I also added a project translator for Core Projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# -> VB Specific to C# -> VB conversion enhancement
Projects
None yet
4 participants