Add range to MarkdownParagraph and MarkdownSpan

[AviAvni]

this is adding only the line to the range please review before adding more information
issue: #410
@tpetricek

[ljw1004]

It looks like you're storing the range exactly how I'd want in the datastructures, and passing around the codebase how I'd expect.

Although: I hope you store range information solely with a character offset into the file. It feels like line/col information is derived from that. I suspect that a lot of book-keeping will be easier if the primary data structure is character-offsets. It's also how Roslyn does it :). (There'd probably be an additional datastructure generated during parsing which is the "character offset at which each line in the file starts", for an easy OffsetToLineAndCol function).

Presumably you're planning to add full range information next, i.e file-offset of both the start and end of each construct? I guess you'd have to start by abandoning ReadLines() and switching to your own ReadLinesWithRangeInformation(). And plumbing through ParseParagraphs to store start+end. And altering ParseChars so it knows which character it's on.

It's a change that has lots of tendrils everywhere, isn't it?!

[AviAvni]

@ljw1004 thanks about the comment I'm doing it as a thanks to your great job in Roslyn
It was the first time I saw this code so to know more how it's works I start implement it without doing big changes to the original code which read lines only add the needed information
Your suggesting do the parsing differently I'll discuss with @tpetricek what the best way

[tpetricek]

I think the S suffix on the active patterns is not very idiomatic. Could we instead have two modules, say String and StringPosition so that the patterns that work on string with position are all in StringPosition module?

[AviAvni]

Done

[tpetricek]

Sorry for the long delay - I finally had a look at this.

• I had one style comment, which is that the active patterns ending with S suffix do not fit well with the F# coding style - I propose to instead have String and StringPosition module that contain two versions of the patterns.
• I got the impression that the current code tracks just the line numbers. I think that's not optimal - we need either line and column numbers, or offset in characters from the beginning of the file. Also, I think range should have start position and end position.
• While we're doing this breaking change, it would be nice to add names on the DU cases - this way, future changes won't be breaking :)

[ljw1004]

I don't have further technical comments, Avi, but just want to say how awesome it is that you're doing this.

[AviAvni]

@tpetricek @ljw1004 Hi thanks for reviewing the code
I Implement all the issue please review again

[tpetricek]

Out of curiosity, is this a needed change for some reason? (Happy to delete the parameter if it's not needed, but then we should probably delete the whole line 26.)

[AviAvni]

I didnt removed the parameter from the method declaration when I looked at all scrpts to see if I need to change something I saw error on this line so I change it

[tpetricek]

@AviAvni Thanks very much for the revised version, it is a fantastic work! It's great to see how this is turning into a great F# Formatting improvement.

I think we are almost there - I made a few more small suggestions, but I think this is now almost at a stage where we can merge the PR.

I added a few nitpicking comments about the naming (because that's the bit which we cannot change once we merge the PR), but aside from that, there are really just smaller things.

Thanks a lot for putting all the effort into this - it is a fairly big change, so I'm sorry it is taking that long, but once it's in, it will be an amazing improvement :) ❤️

[ljw1004]

+1

[AviAvni]

@tpetricek @ljw1004 thanks for the review I made the changes you asked
you can see the comment about the issue of removing the parameter from CodeFormat.CreateAgent

The the issue of adding helper functions for manipulating MarkdownRange and ParsingContext
when I first implement it also to me it look like I need something like that but after I finish the implementation also like you think maybe it's ok like that

any way I thought that after finish with this I'll try to implement the parser with parser combinator library then handling the ranges will lbe more simple so to me it doesn't matter

thanks for helping and letting me do the implementation it's a great library
and it's great that I can help both of you

[tpetricek]

This is now released as version 3.0.0-beta01 (merged into #417, but not in master yet).

@AviAvni Thank you very much for working on this (and for patience with me :-))

@ljw1004 It would be great if you could try the beta version and let us know if everything works as expected. The new version also has named fields on discriminated union cases, which might make the VS interop nicer.