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

[WIP] Service formatting #3542

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
@vasily-kirichenko
Copy link
Contributor

vasily-kirichenko commented Sep 2, 2017

No description provided.

vasily-kirichenko added some commits Sep 2, 2017

@KevinRansom

This comment has been minimized.

Copy link
Contributor

KevinRansom commented Sep 3, 2017

@dotnet-bot test Ubuntu14.04 Release Build please

@KevinRansom

This comment has been minimized.

Copy link
Contributor

KevinRansom commented Sep 3, 2017

@vasily-kirichenko, @dungpa @dsyme

I assume that Phan is going to continue with the Fantomas repo being the main repository for maintaining the Fantomas code. So we need a way to ensure that any updates that appear in this repo, can easily flow back to Fantomas, and that Fantomas changes can easily flow here.

One thing that we did to make FsLexx and FsYacc easy to handle was to publish a nuget package with the Lexx/Yacc source code which we suck in.

Alternatively we could just use Fantomas from a dll from a nuget package.

I welcome any suggestions, I love the addition of the functionality but I don't want to make Phan's life more difficult.

Kevin

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 3, 2017

@KevinRansom Fantomas depends on FCS nuget, so we cannot use it via package reference. Secondly, its API consumes FSharpChecher, which is not allowable in this repo. About Phan, I'm pretty sure he's not been doing any F# for a couple of years. So if anybody wants to do it another way, I'll close this PR.

@KevinRansom

This comment has been minimized.

Copy link
Contributor

KevinRansom commented Sep 3, 2017

@vasily-kirichenko thanks for the info, checker being internal, is pretty much a show stopper against using a third party dll. So I agree grabbing the source is kind of the only way to make it work. However, the Fantomas repo is still active, we should consider how we want to handle that fact. In the mean time, we should certainly move forward with this approach.

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 3, 2017

If this code becomes a part of FCS and FCS is published right from this repo, the Fantomas repository can be deprecated because everyone who uses Fantomas necessarily references FCS as well.

@@ -0,0 +1,24 @@
// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information.

This comment has been minimized.

@forki

forki Sep 3, 2017

Contributor

This line is hilarious ;-)

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Sep 3, 2017

Author Contributor

I've just copied this from another file.

This comment has been minimized.

@KevinRansom

KevinRansom Sep 4, 2017

Contributor

@vasily-kirichenko,

The Apache Licence requires us to retain the original copyright notices of this code and identify prominently any files that we changed.

So we should drop the copyright notice additions, and replace them with the original copyright notice if one existed.
Any files we changed we should add a comment at the top of the file stating that we changed the file.

I appreciate that many projects don't really follow those practices, but we should strive to minimize the risk to this repo, and products built from it.

Snipped from Apache License:
You must cause any modified files to carry prominent notices stating that You changed the files; and You must retain, in the Source form of any Derivative Works that You distribute, all copyright, patent, trademark, and attribution notices from the Source form of the Work, excluding those notices that do not pertain to any part of the Derivative Works; and

Thanks

Kevin

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Sep 4, 2017

Author Contributor

Done. I removed the MS copyright and added the following comment:

// Copied from https://github.com/dungpa/fantomas and modified by Vasily Kirichenko

I'm not sure I should add my name though.

/// Make a position at (line, col) to denote cursor position
let makePos line col = mkPos line col

/// Make a range from (startLine, startCol) to (endLine, endCol) to select some text

This comment has been minimized.

@forki

forki Sep 3, 2017

Contributor

We should add notes to eventually use the existing versions of this

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Sep 3, 2017

Author Contributor

I've not reviewed the code. A quick look at it left an impression that it needs a serious refactoring.


and genImpFile astContext (ParsedImplFileInput(hs, mns)) =
col sepNone hs genParsedHashDirective +> (if hs.IsEmpty then sepNone else sepNln)
+> col sepNln mns (genModuleOrNamespace astContext)

This comment has been minimized.

@forki

forki Sep 3, 2017

Contributor

This is not a common operator right? We should add a note to get rid of it eventually

This comment has been minimized.

@forki

This comment has been minimized.

Copy link
Contributor

forki commented Sep 3, 2017

Wow. I think this would be epic if this would be happening.

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 3, 2017

@KevinRansom how to verify locally that FSharp.Compiler.Private compiles successfully for .net standard 1.6 (or .net core 1.1 or whatever is used for CI build 4)? Or better, how to build the CI part 4 locally?

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 3, 2017

OK, it's ready.

@vasily-kirichenko vasily-kirichenko changed the title [WIP] Service formatting Service formatting Sep 3, 2017

@KevinRansom

This comment has been minimized.

Copy link
Contributor

KevinRansom commented Sep 4, 2017

ci_part3, builds and tests the coreclr which includes FSharp.Compiler.Private.dll for coreclr.

at the root of the depot type: build ci_part3

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Sep 5, 2017

Could we talk through what our criteria would be before integrating this? e.g.

  • can format all compiler code and still bootstrap?

  • visual check of large amounts of formatted compiler and VS code - would we use this on our own code??

  • can format all tests\fsharp... code and still pass tests (that's harder, because many tests test line numbers, still, there are lots of whacky tests under there)

  • we would have to go through all known issues at https://github.com/dungpa/fantomas/issues and consider if they are blockers?

  • 3 approving code reviewers having done detailed reviews?

  • at least 2-3 people saying "we understand the code and could guide people about how to fix bugs in it"?

I'm just aware that we're taking in a very large amount of not-authored-by-any-of-us code. with a set of known issues. Also, this is code with high user exposure: users very much expect to be able to format entire source files, that the formatting will look "right" under default settings, that the code will still work, and that bugs will be fixed if there are problems.

There are many, many positives here - I really want this integrated into FCS - it is a huge addition that is vital in the long term. And from an engineering perspective the testing for the code has been good (both live user testing in VFPT and in other code editors, and good unit testing)

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 5, 2017

can format all compiler code and still bootstrap?

I doubt it will be possible anytime in the future.

visual check of large amounts of formatted compiler and VS code - would we use this on our own code??

The results are usually unacceptable. A lot of options must be added to make it usable on complex code.

can format all tests\fsharp... code and still pass tests (that's harder, because many tests test line numbers, still, there are lots of whacky tests under there)

No way I'm afraid.

3 approving code reviewers having done detailed reviews?

I personally have not read the code at all, just made it compiled.

at least 2-3 people saying "we understand the code and could guide people about how to fix bugs in it"?

Not me, definitely. Fantomas is one-person project, nobody understands the code after @dungpa left F#.

In short, it's a black box code which is better to not be changed in the future. But it's like 1_000_000 times better than no formatter at all. The reality is that nobody is going to implement an alternative formatter, so the best thing we can do is using what we have. On a positive note, the compiler codebase is also one-person project (@dsyme) and we somehow live with it.

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Sep 5, 2017

What I'm looking for from the community here is a credible path to co-ownership and maintainability. I'm not looking to reject this feature - I'm looking to accept it, but in a credible, maintainable, believable way. I believe in the learning and engineering capabilities of the F# community, and I also believe that the community cares about this a lot. Let's find a path.

We can also tune the delivery of this candy. For example, it could be a disabled-by-default with a dialog "user beware, please willing to help fix bugs in this feature if you use it" when first activate it. That would at least allow us to remove it at a later point.

at least 2-3 people saying "we understand the code and could guide people about how to fix bugs in it"?

I personally believe I understand the basic technique used (transform AST, reapply comments in token stream matching), having discussed with @dungpa during its creation, though I haven't read the code to check how it evolved.

But one person with "a vague understanding" is not enough. We need more people to be involved here. I think those who want the feature accepted (which includes me) will have to recruit them and work together on it.

On a positive note, the compiler codebase is also one-person project (@dsyme) and we somehow live with it.

Yes, but.....

  1. I am still involved in maintaining and extending the code, and - barring being hit by a bus or otherwise unable to contribute - everyone knows that I am personally committed to doing so on a long term basis, and
  2. I am also willing to mentor and help others to also contribute and fix issues, and
  3. There are quite a few people who are willing to contribute and fix issues, up to capability, and
  4. The code is very well tested, both unit and field, and
  5. The known-issue count in the F# compiler is very low (just a handful of "Fix Soon" issues, and about 50 "Fix Later" issues, if you spot one is mis-classified, let us know, sometimes we get these classifications wrong), and

These factors matter :)

I'll start to review the code and may send PRs to your branch with additional comments capturing my understanding of how it works.

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 5, 2017

I'll start to review the code and may send PRs to your branch with additional comments capturing my understanding of how it works.

Maybe it's better to disable the service (i.e. comment out the export attribute) and merge?

@forki

This comment has been minimized.

Copy link
Contributor

forki commented Sep 5, 2017

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 5, 2017

@dsyme dsyme changed the title Service formatting [WIP] Service formatting Oct 4, 2017

@nojaf nojaf referenced this pull request Oct 4, 2017

Closed

Add support for F# #269

@enricosada

This comment has been minimized.

Copy link
Contributor

enricosada commented Oct 10, 2017

@dsyme @KevinRansom @vasily-kirichenko while using directly Fantomas package is not possibile (because deps etc), is not the only solution to use a dependency.

Maybe is possibile to use source code files directly.
like adding these needed files as paket github deps.

It doesnt complicate at all the build script at all (is just a command to download file from github), so the fsproj build the downloaded source files, like where in this repo.
The fsproj doesnt need to know at all paket is used to download the files because are just normal local source file, in another dir

Same for tests,

this is good ihmo for maintanability, because with minimal #ifdef (if needed), the code may continue to live in fantomas repo.
So maintain specific bug tracker, faster to evolve/build/contribute etc and this repo/project doesnt become a lot bigger than it is.
From time to time, this repo can bump the specific commit used, but is not complicated like merging VF# <-> FCS.

I know is easier to duplicate code here, and maybe replace fantomas directly, but add a lot of burden here.

If is ok as idea, i volounteer to update this PR/fantomas code (with other PR)

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Oct 10, 2017

@enricosada I think it's either all or nothing - we either fix the bugs and get this up to scratch for inclusion in FCS (and deprecation of Fantomas), or we don't integrate. There's no point in doing source inclusion until the quality is high enough.

I'd prefer the first path. If in the interim we need to make changes to this PR to allow ongoing integration from Fantomas fixes --> this PR then yes, we should do that.

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Oct 10, 2017

@enricosada Are there specific changes coming into Fantomas that make you want to move to source inclusion? Just wondering, thanks

@realvictorprm

This comment has been minimized.

Copy link
Contributor

realvictorprm commented Oct 10, 2017

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Oct 18, 2017

I'm also working with @odytrice on this in the mentorship program hope we have something soon.

@AviAvni How it's going? Any progress?

@rkjellin

This comment has been minimized.

Copy link

rkjellin commented Oct 18, 2017

I tried to add support for struct tuples in fantomas (I think I succeded but not sure if the fix should be done here or on the fantomas repo?) and found the following bug:
let x = fun ((u, v):(int*int)) -> 5
is formatted to
let x = fun (u, v) : int * int -> 5
which is a syntax error:
Error FS0010 Unexpected symbol ':' in lambda expression. Expected '->' or other token.

@AviAvni

This comment has been minimized.

Copy link
Contributor

AviAvni commented Oct 18, 2017

@vasily-kirichenko I helped @odytrice with how to debug this and help him to understand how this is work
the mentorship is over for us we did all the 6 sessions now he still debugging some issue
I don't have much time to work on it my self now
from the little i debug i found that the function collectComments https://github.com/Microsoft/visualfsharp/pull/3542/files#diff-041b15d051e5241912c1176ebf9f42e8R121
not collect line comments correctly something in the active patterns not good did't explore it
@odytrice please share you updates too

@odytrice

This comment has been minimized.

Copy link

odytrice commented Oct 20, 2017

@AviAvni @vasily-kirichenko Still on it. Will send a PR soon.

Like @AviAvni said
I am currently working on issue fsprojects/fantomas#204 was able to narrow the issue down to TokenMatcher.fs integrateComments line 423.

It's been a bit more challenging as I had imagined but I will soon be done with it

@odytrice

This comment has been minimized.

Copy link

odytrice commented Nov 4, 2017

Just sent a PR to the Fantomas Repo. The specific issue was way more difficult than I anticipated. Details here fsprojects/fantomas#217

@realvictorprm

This comment has been minimized.

Copy link
Contributor

realvictorprm commented Dec 5, 2017

@vasily-kirichenko all the code generation stuff must be moved out of the VS folder, it's for the compiler (service). So you prefer PR's to your branch or do you want to give me access to it?

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Dec 5, 2017

@realvictorprm Neither 'coz it's in the right folder. I recommend you to learn how this whole repo is organized.

@enricosada enricosada referenced this pull request Jan 19, 2018

Closed

SharpFormat #637

5 of 5 tasks complete
@nojaf

This comment has been minimized.

Copy link

nojaf commented Mar 14, 2018

Hi all, as part of the mentoring program @AnthonyLloyd (mentor) and I would like to contribute to this feature. We started last week by solving an issue in the fantomas repo. The idea is that we solve a couple of more of these in the weeks to come.

Are there any particular issues we should focus on first? And could we perhaps start a slack channel for if we have questions about the source code?

Lastly could somebody explain the benefits of having the formatting in the FCS versus having it as a dotnet 2.1 global cli tool?

@s-trooper

This comment has been minimized.

Copy link

s-trooper commented Mar 14, 2018

you could start with most commented first :-)

one possible benefit of having the formatting in the FCS, is that it can run in process. that would be significant (i look at u ngen) faster then spawn new processes. of course IDE plugin implementation matters.

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Mar 14, 2018

@nojaf My recommendation is

  1. Start a new PR (possibly based on this one) and keep a running, up-to-date record of the remaining work required for this to be accepted

  2. One criteria is that we be able to format all the files in the compiler without breaking the semantics of code (some bad formatting is ok), see #3542 (comment). So we have to work through all those issues one by one

  3. It may be worth bringing in Fantomas via a git sub-module in this PR 1nitially so we can co-develop these features. I don't use git submodules much but they are effective for this kind of situtation

  4. Tag and prioritize all the issues in the Fantomas repository. Clean out any old or unimportant ones.

@KevinRansom

This comment has been minimized.

Copy link
Contributor

KevinRansom commented Sep 12, 2018

@dsyme, @vasily-kirichenko, @cartermp

it looks to me as if we do not have faith that this is going to get any love. Do you have any objections to this PR being closed, and we await a new clean PR when someone gets around to adding formatting to FCS?

Thanks

Kevin

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor Author

vasily-kirichenko commented Sep 12, 2018

I don't care about this PR anymore 'coz there have been many PR merged into Fantomas since this PR and it works as a separate dll in Rider nicely, so I'm ok to kill this one.

@nojaf

This comment has been minimized.

Copy link

nojaf commented Sep 12, 2018

Hi @KevinRansom , Fantomas is getting love.

https://twitter.com/verdonckflorian/status/1039951854904508418

She is just not ready yet for a relationship with the FCS.
Maybe someday.

@KevinRansom

This comment has been minimized.

Copy link
Contributor

KevinRansom commented Sep 12, 2018

@nojaf, yes and I am glad she is.

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Sep 19, 2018

@nojaf @KevinRansom Probably best not to use "she/love/relationship" language to describe technical work in this repo. I used to use that kind of language and likely still do from time to time, I know it's harmless, but it can lead to imagery that some find uncomfortable. Not in this case particularly though.

@s-trooper

This comment has been minimized.

Copy link

s-trooper commented Jan 23, 2019

I just wrote my own source code formatter, may be it can be useful.

P.S: sorry if that the wrong place for that announcement but i suppose people here are interested in such things

@smoothdeveloper

This comment has been minimized.

Copy link
Contributor

smoothdeveloper commented Jan 23, 2019

@s-trooper thanks! I think those monitoring the issue / repository will be glad to know about your project.

@cartermp I believe the out-of-box story of F# VS editor wrt to autoformatting is significant, I've encountered colleagues that drew attention to this (versus say C# VS editor with or without Resharper) when I brought the idea of using F#.

Any kind of progress in that direction is IMHO going to help with language adoption and VS users to be (even) happy(ier) editing F# code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment