search

fsharp / fslang-spec

F# Language Specification
MIT License
21 stars 1 forks source link

Chapter 1 - Introduction #4

Closed Martin521 closed 4 months ago

Martin521 commented 6 months ago

This is chapter 1 (Introduction) Please review and merge the infrastructure first, then the chapters.

edgarfgp commented 4 months ago

Maybe we can ask @abelbraaksma to help us review all this PR’s.

vzarytovskii commented 4 months ago

Already did on slack, he will get to it once he has time for it

abelbraaksma commented 4 months ago

Indeed, I will, tx @edgarfgp 👍

abelbraaksma commented 4 months ago

If we are in agreement with these changes, I can also just make them for you, but I think that requires a separate PR, as I don't have access to your forked branch.

Martin521 commented 4 months ago

If we are in agreement with these changes, I can also just make them for you, but I think that requires a separate PR, as I don't have access to your forked branch.

Thanks for the thorough review. I will look at the "opt" issue next.

abelbraaksma commented 4 months ago

@Martin521, great that you're tackling this. About b6d8fd4 and the like: I didn't create a comment for each occurrence of "top-level code should not be indented". This is probably something that ought to be changed for all code occurrences, across the PRs.

Did you use a script for this? Cause then a change to that script would suffice, hopefully :).

This (format fsi examples) 3759ee0, is the same: it should be applied everywhere, if possible.

Martin521 commented 4 months ago

@Martin521, great that you're tackling this. About b6d8fd4 and the like: I didn't create a comment for each occurrence of "top-level code should not be indented". This is probably something that ought to be changed for all code occurrences, across the PRs.

That's already done for chapter 1. And it seems in all other chapters we did not do this top-level indent, so everythings seems to be ok.

Did you use a script for this? Cause then a change to that script would suffice, hopefully :).

Formatting the code examples was a lot of manual work by Edgar and myself. The original conversion swallowed all indentation and often cut the code block into pieces (e.g. here).

This (format fsi examples) 3759ee0, is the same: it should be applied everywhere, if possible.

Also done :-)

abelbraaksma commented 4 months ago

wow, great work! I'll go over this one more time, see if we've caught everything (and otherwise, there's always another day, right?).

Btw, can we strategize about changes that aren't the same as the source? My take in this review is the following:

I think, unless we stick to this, it would become a much too large effort in the first go (i.e. there were many places where I noticed mistakes in the original text, but I decided to let it slide as to not muddy the main conversion process).

Edit: @vzarytovskii wrote this above (https://github.com/fsharp/fslang-spec/pull/4#discussion_r1614176017) on a suggestion to include links/hrefs:

I think we want to first merge as is, and then make a branch/tag with new more fresh version, so there are no surprises if we want to review changes, etc.

which touches on these points directly. So yes, I agree, let's do that.

abelbraaksma commented 4 months ago

You wrote (https://github.com/fsharp/fslang-spec/pull/4#discussion_r1616901106):

I agree it looks nicer as code block, but we have reserved the fsharp info string for F# code that we could later extract and verify (like the C# spec does or at least did it). See also the conversion guideline that we used. I will later go separately through the code and turn the fsi examples into code blocks with the fsother info string. (As, I believe, I did in later chapters.)

On this point: the FSI examples are prime examples that can be auto-verified (i.e., parse the string after > and see if the result is the same as the shown output). But if we call it fsother or fsi (which can then still be auto-verified), perhaps we can make that an alias for f# coloring (or, better, perhaps we can make fsi a proper tag so that it recognizes >? But that's a future scen)

Martin521 commented 4 months ago

You wrote (#4 (comment)):

I agree it looks nicer as code block, but we have reserved the fsharp info string for F# code that we could later extract and verify (like the C# spec does or at least did it). See also the conversion guideline that we used. I will later go separately through the code and turn the fsi examples into code blocks with the fsother info string. (As, I believe, I did in later chapters.)

On this point: the FSI examples are prime examples that can be auto-verified (i.e., parse the string after > and see if the result is the same as the shown output). But if we call it fsother or fsi (which can then still be auto-verified), perhaps we can make that an alias for f# coloring (or, better, perhaps we can make fsi a proper tag so that it recognizes >? But that's a future scen)

True. BTW, there are only the few fsi examples in this chapter and one more in one of the other chapters. So, very few places to change if we decide at some point to do something like this.

Martin521 commented 4 months ago

Looking good!!!

Now that I understand the post-conversion ideas a bit better, perhaps consider this for the (fake) abnf sections? It may help with any later automation we may need to do.

Oh, and one tiny nit. Otherwise, good to go. I'll mark approved already, these aren't blockers.

All the grammar pieces in the other chapters are marked 'fsgrammar' (which we can easiliy replace with something like abnf later if we want so). I did not mark these definitions here that way because they are not really F# grammar but sort of the grammar of the grammar (or, as the heading says, "notational conventions").

So, if it is ok for you, I will leave this as is.

EDIT: I take this back. the first three items are actually examples of F# grammar. I will tag them 'fsgrammar' to be consistent with all the other chapters.

Martin521 commented 4 months ago

I also added the chapter in spec/Catalog.json so that the build script can process it.

abelbraaksma commented 4 months ago

Great work! @vzarytovskii this is ready. I don't (yet) have the ability to merge, can you, or someone else with merge access, merge it?