Skip to content

Run code formatter on VB compiler source code - #1140

Closed
jaredpar wants to merge 1 commit into
dotnet:masterfrom
jaredpar:format-vb
Closed

Run code formatter on VB compiler source code#1140
jaredpar wants to merge 1 commit into
dotnet:masterfrom
jaredpar:format-vb

Conversation

@jaredpar

@jaredpar jaredpar commented Mar 9, 2015

Copy link
Copy Markdown
Member

This is the initial run of the code formatter on the VB compiler source code. Further runs I'll expand on this to include all VB code in a future run.

This is the initial run of the code formatter on the VB compiler source code.  Further runs I'll expand on this to include all VB code in a future run.
@jaredpar

jaredpar commented Mar 9, 2015

Copy link
Copy Markdown
Member Author

FYI @dpoeschl

@gafter

gafter commented Mar 10, 2015

Copy link
Copy Markdown
Member

Paging @AlekseyTs as I know you've had some strong opinions about some of these changes in the past.

@AdamSpeight2008

Copy link
Copy Markdown
Contributor

@AlekseyTs isn't going to be happy, having the scroll horizontally to see the changes.
I think it also changes the scoping of some the methods. From not having an explicitly declared one (so defaults I think Private?) to having an an explicitly declared one.

@AdamSpeight2008

Copy link
Copy Markdown
Contributor

Please also consider using a 2 space indentation, instead of 4. It reduce the sideways scrolling on diffs.
My Fork's scanner folder uses. (Note: I also turn of the pretty-printer)

It's also going cause a lot of merge conflicts, that wouldn't to deal with. I think it is likely afterwards that most would re-clone the repo rather than deal with them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It feels like the original indentation should be preserved.

@AlekseyTs

Copy link
Copy Markdown
Contributor

LGTM with exception of several unexpected indentation changes (each has a separate comment).

@jaredpar

Copy link
Copy Markdown
Member Author

@AlekseyTs thanks for taking the time to review that ... giant change ;)

CC @basoundr, @dpoeschl for the indentation issues. I agree that a lot of those look like bugs, in particular the comment shifting. I'm going to manually undo these before I commit but it's important that we fix the Formatter.FormatDocumentAsync code as well. Is there a bug tracking this already?

@basoundr

Copy link
Copy Markdown
Contributor

@jaredpar I remember we ran into some formatting issue sometime back when you ran Formatter.FormatDocumentAsync (couple of weeks, maybe?). I dont see any issue that tracks that. We must definitely track this issue.

I can create one and will reference this PR in the bug, if you dont remember filing an issue already.

@jaredpar

Copy link
Copy Markdown
Member Author

@basoundr I'll get a bug filed as a part of checking this in.

@basoundr

Copy link
Copy Markdown
Contributor

@jaredpar Thanks :)

@jaredpar

Copy link
Copy Markdown
Member Author

Formatting issue is #1177

@jaredpar jaredpar closed this in 475f6d7 Mar 10, 2015
@jaredpar
jaredpar deleted the format-vb branch August 21, 2018 01:00
dibarbet pushed a commit that referenced this pull request Aug 18, 2026
Fix incorrect containing type usage in FixAllContextExtensions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants