-
Notifications
You must be signed in to change notification settings - Fork 64
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
Allow comments after explicit line continuations #65
Comments
See related: #41 |
It looks terrifying but we still want it. I believe Lucian sent an email saying he wanted to be able to write the text of war and peace in the margins of his code. |
It's been over a year, what does Approved-in-Principle actually mean? Is any work being done to increase where I can put comments? There are several Open issues around comments and some are non-breaking. Maybe there is a way to open up a discussion around non-breaking ways to add comments (and blank lines) in more places. I saw that changing where comments are attached was closed but allowing comments after an _ could serve the same purpose and would be non-breaking. |
@paul1956 approved in principle means the LDM thinks it is an idea worth pursuing and likely doing. I am not aware of anyone in particular doing anything to make it happen (including you nor me). This is the right place to discuss it. If you want to move it forward I recommend you announce that you’re going to work on a prototype and do so (as a PR in the Roslyn repo). |
@gafter as I have stated before I don't know enough about GitHub, a PR or where even to find the actual VB compiler to attempt it. If someone wants to point to the parser source or a place to start I would be very interested in working on it. I still see Roslyn as a very big black box with source, and developer rules/controls but not a lot of instruction on what to do with it. As compared to the Analyzers and Refactoring's which have well documented examples and words/whitepapers that explain the concepts. I have just completed a C# to VB translator 99% in VB that can convert all of Roslyn to VB, parse without errors, and compile and I only comment out a few things that have no way to be represented in VB (most Unsafe code), I still need to address nested Modules, ERRID.ERR_ModuleNotAtNamespace, so for now I ignore it and ERRID.ERR_StandaloneAttribute), and I preserve all comments and most formatting, but some comments are so far from where they belong they are useless or misleading. It is these comments and ERRID.ERR_StandaloneAttribute I want to address. |
@gafter, thanks I will look at it. Cloning, restoring and building Roslyn is a lot easier than the last time I tried. Any place to find tips on building the VB Compiler and testing my changes as well as any design documents/whitepapers for first time developers? |
@gafter is there a preference on Syntax I want to tackle this case first and then do _ comment on a blank line
Or
|
I personally think the version with a space is better. Reduces the possibility of confusing it with an identifier. |
Space wins. As it turns out this is "trivial", the entire change is about 25 lines in scanner.vb, about 6 new lines and a few lines moved around and 2 new tests need to be added. All the infrastructure already assume this was a feature. My next question is how to write a PR. I am using VS 2017 enterprise and did use GitHub to add the project and my changes show up with red Checks next to them. |
@reduckted I have read all that before but none of it really help someone who has never done it before. To start I cloned Master, do I now branch or make the changed then branch? I have saved the changes on my local system and reverted back to master. I don't even know what Jenkins is, what I really need is to get to "feature is championed and validated by LDM a developer will be assigned to help". I am sure once I get through the first one, other should be easy. |
As a member of the LDM, I hereby champion this proposal. @jaredpar We should assign someone to help this external developer, please. You should work on a "branch". If you're using the git command-line, you create a branch locally by doing something like
where
Once you've done that you're ready to submit a pull request for us to review. |
I am using Visual Studio 2017 Enterprise and have Roslyn cloned on my local system where I have made and tested the change, backed up and undid the 2 changed files. How does the "git checkout" know what and where it is checking out from? After I branch how do I tell VS to open my branch instead of the original. Sorry for what seems like silly questions but it is been decades since I worked on anything as part of a team and then we used Visual Safe Source. |
This is a great opportunity for community!!! @AdamSpeight2008 or @AnthonyDGreen or anyone else. If you can walk @paul1956 through the process and create any docs along the way (for example, I always work on a fork, not just a branch, and what the heck is the difference). It is an intimidating process. (As an aside, some people have their mom call about how to fix the DVR or some email feature. My kids still get a call when I have a pending "git-astophy" - git catostrphy) To me, part of what it means to have a community here is for people to help each other working with our git and our systems. |
@paul1956 I personally use TortoiseGit, than Visual Studio for git stuff mostly along with VS Command Prompt.
Adding a Feature (unofficial)
|
@AdamSpeight2008 if we could start simple, I have forked Roslyn by clicking Fork Icon on GitHub/Roslyn website and now I have paul1956\Roslyn I don't see any way to add my name after Roslyn on the website. Then I cloned it to my local machine using Open in Visual Studio where I was given an opportunity to change name. Git Fetch All says 'All' Does not appear to be a repository. My fork only has 15 files/directories in the root while Master has 26, all the CMD files are missing as is Compiler.SLN (the thing I am working on) |
@AdamSpeight2008 After cloning, my Roslyn looks nothing like the original, restore, build and test commands don't exist. Watching the video was interesting but didn't explain why my fork looks nothing like Roslyn. |
@paul1956 Looks way out of date, 3 years at least. |
@paul1956 you will need to pull in the latest changes from the main roslyn branch into your fork. |
@jmarolf I did a fork yesterday from https://github.com/dotnet/roslyn and the image above is what I got. I have been told to pull lastest or rebase. I did the first from my PC to my local Repo and nothing changed, and Google and Video don't help with how to Rebase. I did the development by downloading a zip file with Visual Studio from https://github.com/dotnet/roslyn and then Forked Roslyn and said open in Visual Studio, my fork does appear 3 years old but my Zip is current, this doesn't seem like my issue. Exactly how to I pull in latest changes into https://github.com/paul1956/Roslyn or how do I rebase? |
@AdamSpeight2008 the image is from a fork I did yesterday. |
@AnthonyDGreen @KathleenDollard @paul1956 |
Thank you @paul1956 and @AdamSpeight2008! I think not on the REM comments. I could be talked out of that position if there are compelling arguments for it. "We might as well" doesn't seem a compelling argument and it's the only one I have thought of so far. |
@KathleenDollard I'm only asking as the VB Grammar, permits both as a CommentMarker.
|
@AdamSpeight2008 Right now I am only working on _ ' Comment. I did not know REM existed and after I saw that it did I tried to use it to address my original issue of being able to put comments in more places and unfortunately Rem comments didn't solve my issues (I was hoping I could put a REM on a blank line and have it ignored but that is not the case. I hope eventually _ ' Comment on a blank line allow it to be ignored but this may take a lot of work. I use ' but the code works with any SingleQuoteCharacter. |
I have only ever thought of PS: In all my 15 years of writing VB code, I've never commented with REM. Neither have I met code originally written in VB.NET that uses REM as the comment marker. |
The implementation doesn't distinguish On the practical side, I do use |
Right now Rem is not allowed after an _. What is allowed is _ followed by 0 or more spaces followed by ', then everything till the end of line is a comment. Adding Rem would be possible but would require a lot of additional tests and some changes to the scanner. |
Oops. I confused the suggestion. Thought this was modifying an existing case with implicit line continuation for some reason (maybe because I was reading #232 at the same time). This is a completely new case, so the existing code paths don't really matter. I would guess that it would mostly just double the number of new tests replacing ' with REM but there might be more stuff around expand/reduce in the IDE I'm not thinking about--I legit don't know. Random thoughts on REM:
Dim q = From x
' Comment
In y
REM Comment
Select x This was more the scenario I think I was thinking about before. I suspect it'll use more of the existing codepaths for scanning (just a guess). Then it would be weird if REM just works in some new cases but not others just based on whether existing code was reused or not.
|
If the VB LDM would allow it, I'd be happy to submit support for REM in a separate follow-up PR, so as not to hold this one up. |
@AnthonyDGreen I would love to allow comments on any "blank Lines" with Explicit or Implicit line continuations C# already allows this. I have suggested that several different ways. Allowing Rem after _ ' is something completely different and I can look at it or you can but I though @KathleenDollard did not see the value and I agreed. |
@AnthonyDGreen I looked at supporting Rem, I thought is would be a very trivial change just replace IsSingleQuote(Peek(Here)) with PeekStartComment(Here) > 0 but several tests fail beyond just the tests that were expected to fail and now pass and multiple test support routines that "rem" tests use don't support passing in a compiler version. |
@KathleenDollard It would be good to create a 16.0 milestone and attach this feature, since @paul1956 is working on implementation. Thanks |
The feature was merged for dev16.1 preview2 (dotnet/roslyn#28327) which includes a preview of VB 16. |
I am closing this, since this feature is now merged and scheduled for the next update to Visual Studio 2019, which is now in Preview. It has been a long process and massive thanks to @paul1956 for this work!!! |
Solved in project Mercury |
Has this regressed in 16.5.4? |
I don't think so but some installations like Roslyn explicitly set VB version to 15.5 in a settings file and some project files also do that. |
Currently, VB allows the use of comments after implicit line continuations:
However, it doesn't allow comments after explicit ones:
Personally, I greatly prefer having the operator at the beginning of a line, as it is an additional visual cue that I'm chaining method calls or writing a very long expression, as opposed to breaking up a very long method call into separate lines for each argument.
The text was updated successfully, but these errors were encountered: