Simplify, rename, refactor Ident parsing #266

7sharp9 · 2013-12-20T12:39:57Z

Ive opened this pull request so we can track the refactoring left to do on the Ident parsing.
It also means that anyone can work on this by submitting a PR to this branch.

Next step is to review this code to see if there are any other simplifications we can do before moving it to the FSharp.CompilerBinding project.

7sharp9 · 2013-12-20T12:45:06Z

Just noticed that GetDeclarations has some parser functionality inside it too, this could do with being extracted out. @rneatherway Do you have a similar function in autocomplete that duplicates this too?

rneatherway · 2013-12-20T13:23:18Z

If that is where the possible completions are found, then it is at
https://github.com/fsharp/fsharpbinding/blob/master/FSharp.AutoComplete/Program.fs#L219
and
yes there is some custom parser code in there. This is a bit more
complicated to handle the cases with raw idents Don pointed out in #233
This approach should be used by monodevelop as well to handle those cases.

On Fri, Dec 20, 2013 at 12:45 PM, Dave Thomas [email protected]:

Just noticed that GetDeclarations has some parser functionality inside it
too, this could do with being extracted out. @rneatherwayhttps://github.com/rneatherwayDo you have a similar function in autocomplete that duplicates this too?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/266#issuecomment-31007263
.

ghost · 2013-12-20T18:20:27Z

monodevelop/MonoDevelop.FSharpBinding/Services/LanguageService.fs

-
-        let loc, line, col, currentLine, lineStr = preCrack (offset, doc)
-
+    let FindLongIdents (col, lineStr) = 


be consistent with the local style and use camelCase here

ghost · 2013-12-20T18:21:54Z

looks ok to merge

7sharp9 · 2013-12-20T18:48:14Z

master is currently broken, completion lists broken #267, I was hoping to get it fixed while doing this. Ive run out of time for now though.

Ive switched back for 3.2.21 for now.

rneatherway · 2013-12-20T19:07:29Z

Just pushed a fix for #267

On Fri, Dec 20, 2013 at 6:48 PM, Dave Thomas [email protected]:

master is currently broken, completion lists broken #267 #267,
I was hoping to get it fixed while doing this. Ive run out of time for now
though.

Ive switched back for 3.2.21 for now.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/266#issuecomment-31032100
.

… refactorIdentParsing Conflicts: monodevelop/MonoDevelop.FSharpBinding/Services/LanguageService.fs

7sharp9 · 2013-12-20T22:35:45Z

think the fix is merged in, as is the correct camel case. Im not sure where we want to move this to for reuse? new file with a module in, or add to the parser one?

7sharp9 · 2013-12-20T23:22:39Z

@rneatherway I think a made GetDeclarations work the same as you do in AutoComplete, I think you may do some filtering latter on though. You might want to merge that bit in, I don't know exactly if thats a special case for you though...?

rneatherway · 2013-12-20T23:35:10Z

Thanks dave, I'll take a look tomorrow morning. Just doing some family tech
support :-)
On 20 Dec 2013 23:22, "Dave Thomas" [email protected] wrote:

@rneatherway https://github.com/rneatherway I think a made
GetDeclarations work the same as you do in AutoComplete, I think you may do
some filtering latter on though. You might want to merge that bit in, I
don't know exactly if thats a special case for you though...?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/266#issuecomment-31049204
.

Also remove redundant 'currentIdent' which was returned separately. We don't need this anymore and it is included in the identIsland anyway.

Also change fsautocomplete to use the merged parsing code

rneatherway · 2013-12-21T14:28:13Z

Sorry I meant to push to my own repo and then submit a PR to this branch.

In any case, what I did here was to separate out the parsing code from the completions function into another findLongIdents* function. I then moved all three of these functions to the Parsing module and hooked up fsautocomplete to use them as well.

I'm not wedded to these functions being in Parsing, now that they are being used successfully by both projects we can move them around easily.

I'm really pleased we've managed to get this done, that duplication was very unsatisfactory. Hopefully we can factor out more as time goes on.

ghost · 2013-12-21T16:13:18Z

As I understand it this is ready to merge?

rneatherway · 2013-12-21T16:31:43Z

Yes as far as I'm concerned. It passes the emacs/fsautocomplete tests and
everything seems to work in MD.

On Sat, Dec 21, 2013 at 4:13 PM, Don Syme [email protected] wrote:

As I understand it this is ready to merge?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/266#issuecomment-31066242
.

Simplify, rename, refactor Ident parsing

7sharp9 · 2013-12-21T18:09:55Z

Cool! I find it easier when we have a fork in the main repo rather than in a clone, it saves mucking about with multiple remotes.

Simplify, rename, refactor Ident parsing

f404412

ghost reviewed Dec 20, 2013
View reviewed changes

7sharp9 added 2 commits December 20, 2013 21:49

Fixes unnecessary precrack and camelcase

84d8e96

Merge branch 'master' of https://github.com/fsharp/fsharpbinding into…

9d57fab

… refactorIdentParsing Conflicts: monodevelop/MonoDevelop.FSharpBinding/Services/LanguageService.fs

factored out the MonoDevelop types in GetDeclarations

40621fd

rneatherway added 2 commits December 21, 2013 13:37

Separate completion parsing from requests

d7463a3

Also remove redundant 'currentIdent' which was returned separately. We don't need this anymore and it is included in the identIsland anyway.

Move ident parsing into FSharp.CompilerBinding

1ae87ad

Also change fsautocomplete to use the merged parsing code

rneatherway mentioned this pull request Dec 21, 2013

Pressing "." in "0 .. 60" inserts fals completion #255

Closed

ghost pushed a commit that referenced this pull request Dec 21, 2013

Merge pull request #266 from fsharp/refactorIdentParsing

e8f8daf

Simplify, rename, refactor Ident parsing

ghost merged commit e8f8daf into master Dec 21, 2013

7sharp9 deleted the refactorIdentParsing branch January 8, 2014 22:45

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify, rename, refactor Ident parsing #266

Simplify, rename, refactor Ident parsing #266

7sharp9 commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

rneatherway commented Dec 20, 2013

ghost Dec 20, 2013

ghost commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

rneatherway commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

rneatherway commented Dec 20, 2013

rneatherway commented Dec 21, 2013

ghost commented Dec 21, 2013

rneatherway commented Dec 21, 2013

7sharp9 commented Dec 21, 2013


		let loc, line, col, currentLine, lineStr = preCrack (offset, doc)

		let FindLongIdents (col, lineStr) =

Simplify, rename, refactor Ident parsing #266

Simplify, rename, refactor Ident parsing #266

Conversation

7sharp9 commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

rneatherway commented Dec 20, 2013

ghost Dec 20, 2013

Choose a reason for hiding this comment

ghost commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

rneatherway commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

7sharp9 commented Dec 20, 2013

rneatherway commented Dec 20, 2013

rneatherway commented Dec 21, 2013

ghost commented Dec 21, 2013

rneatherway commented Dec 21, 2013

7sharp9 commented Dec 21, 2013