[WIP] Service formatting #3542
[WIP] Service formatting #3542
Conversation
|
@dotnet-bot test Ubuntu14.04 Release Build please |
|
@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 |
|
@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. |
|
@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. |
|
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. | |||
There was a problem hiding this comment.
I've just copied this from another file.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Done. I removed the MS copyright and added the following comment:
// Copied from https://github.com/dungpa/fantomas and modified by Vasily KirichenkoI'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 |
There was a problem hiding this comment.
We should add notes to eventually use the existing versions of this
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
This is not a common operator right? We should add a note to get rid of it eventually
There was a problem hiding this comment.
Yeah, it's defined here
as "function composition".
|
Wow. I think this would be epic if this would be happening. |
|
@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? |
|
OK, it's ready. |
|
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 |
|
Could we talk through what our criteria would be before integrating this? e.g.
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) |
I doubt it will be possible anytime in the future.
The results are usually unacceptable. A lot of options must be added to make it usable on complex code.
No way I'm afraid.
I personally have not read the code at all, just made it compiled.
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. |
|
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.
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.
Yes, but.....
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. |
Maybe it's better to disable the service (i.e. comment out the export attribute) and merge? |
|
I wish we can hear what @dungpa feels about all of this. I hope he can
comment here. This looks like some serious part of his OSS life and we
should be ask him if he's OK with us merging it into FCS. I mean even if
the license allows it, I think that's an important step. Also maybe he is
still up for answering questions or even to review new fixes from time to
time.
So @dungpa if you read this, then please comment ;-)
|
|
@dsyme @KevinRansom @vasily-kirichenko while using directly Maybe is possibile to use source code files directly. 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. Same for tests, this is good ihmo for maintanability, because with minimal #ifdef (if needed), the code may continue to live in fantomas repo. 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) |
|
@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. |
|
@enricosada Are there specific changes coming into Fantomas that make you want to move to source inclusion? Just wondering, thanks |
|
Sorry for disturbing this discussion, I just wonder whether someone noticed
my suggestion on merging source code generation for 3 PR's, including this
PR.
Am 10.10.2017 15:26 schrieb "Don Syme" <[email protected]>:
… @enricosada <https://github.com/enricosada> Are there specific changes
coming into Fantomas that make you want to move to source inclusion? Just
wondering, thanks
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3542 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AYPM8AZT-Y2smyGARDjnpogpqYWMcw3Wks5sq3BugaJpZM4PK7Xz>
.
|
|
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: |
|
@vasily-kirichenko I helped @odytrice with how to debug this and help him to understand how this is work |
|
@AviAvni @vasily-kirichenko Still on it. Will send a PR soon. Like @AviAvni said It's been a bit more challenging as I had imagined but I will soon be done with it |
|
Just sent a PR to the Fantomas Repo. The specific issue was way more difficult than I anticipated. Details here fsprojects/fantomas#217 |
|
@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? |
|
@realvictorprm Neither 'coz it's in the right folder. I recommend you to learn how this whole repo is organized. |
|
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? |
|
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. |
|
@nojaf My recommendation is
|
|
@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 |
|
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. |
|
Hi @KevinRansom , Fantomas is getting love. https://twitter.com/verdonckflorian/status/1039951854904508418 She is just not ready yet for a relationship with the FCS. |
|
@nojaf, yes and I am glad she is. |
|
@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. |
|
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 |
|
@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. |
No description provided.