Skip to content
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

Added F# support and example #491

Merged
merged 6 commits into from
Feb 13, 2015
Merged

Added F# support and example #491

merged 6 commits into from
Feb 13, 2015

Conversation

simon-reynolds
Copy link
Contributor

Added support for F# and included example page

Added F# support and example
lookbehind: true
},
{
pattern: /(^|[^\\:])\/\/.*?(\r?\n|$)/g,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the last group needed? Do we want to consume the line feed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We most certainly don’t. Think of CSS styling that adds a background too. This should be converted to a lookahead. Thanks for catching that Golmote!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last group matches the pattern found in prism-clike.js for single line comments
F# uses the same syntax for single line comments, but the comment property had to copied across as F# uses (* multi-line comment_) instead of the usual C /_ multi-line comment */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked and we are indeed using this in clike. This should be fixed there too…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even a lookahead should normally not be needed. A simple .* should do the job, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, even better!

@Golmote
Copy link
Contributor

Golmote commented Feb 6, 2015

Thanks for submitting this.

(Note for later: this fixes #447 )

'keyword': /\b(abstract|and|as|assert|base|begin|class|default|delegate|do|done|downcast|downto|elif|else|end|exception|extern|false|finally|for|fun|function|global|if|in|inherit|inline|interface|internal|lazy|let|let!|match|member|module|mutable|namespace|new|not|null|of|open|or|override|private|public|rec|return|return!|select|static|struct|then|to|true|try|type|upcast|use|use!|val|void|when|while|with|yield|yield!|asr|land|lor|lsl|lsr|lxor|mod|sig|atomic|break|checked|component|const|constraint|constructor|continue|eager|event|external|fixed|functor|include|method|mixin|object|parallel|process|protected|pure|sealed|tailcall|trait|virtual|volatile)\b/g,
'string': /@?("""|"|')((\\|\n)?.)*?\1(B)?/g,
'preprocessor': /^\s*#.*/gm,
'number': /\b-?(0x)?(\d[a-fA-F]?)*\.?((\d\.?[a-fA-F]?)(y|uy|s|us|l|ul|un|L|UL|F|f|lf|LF|I|M|m))?\b/g
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get this new pattern properly. Why are there two dots? Can hexadecimal numbers use a floating point syntax in F#? If not, I guess \d*\.?[\da-fA-F]+ should be enough.
Maybe you should add a "Numbers" part in the example file so that you can check they are highlighted properly. (Maybe check those listed there, for example?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was getting a bit complex as F# floating point numbers can be declared by ending a number with a dot
I've instead split the numbers regex to handle different scenarios as per commit message. Good idea with the Numbers section in the example file, I've included that as well

…regex to handle hex, binary, integer and decimal numbers separately
{
pattern: /\b-?(\d{1,}(y|uy|s|us|l|u|ul|L|UL|I)?)\b/g
}
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should simplify the 4 patterns here:
1st: ((\d*)|([a-fA-F]*))* => [\da-fA-F]*. And there seems to be a pair of parentheses that can be removed around the whole number.
2nd: ((0|1))* => [01]*. There is also a pair of parentheses that's not needed.
3rd: I guess the f in the middle is for handling the numers ending with a dot? What about... 2.F or 2.m? Are they valid numbers in F#? Furthermore, (E|e) should be simplified as [Ee] and f|F|m|M as [fFmM]. And still a pair of unneeded parentheses.
4th: \d{1,} => \d+. An the pair of unneeded parentheses can be removed.

Try to keep the patterns as short as possible. This will actually make them more readable. Counting the parentheses is probably the hardest part in understanding a regexp ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions

'preprocessor': /^\s*#.*/gm,
'number': [
{
pattern: /\b-?(0x[\da-fA-F]*.(un|LF)?)\b/g
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the dot? Is it supposed to be an actual dot or any character? I don't get this.
Also, I think you should use + instead of * since we want at least one char.
There are still unneeded parentheses around the whole number.
Finally, this page on the MSDN show an example using lf in lowercase, that is not handled here.

@Golmote
Copy link
Contributor

Golmote commented Feb 8, 2015

Concretely I think the number patterns should be:

    [
        {
            pattern: /\b-?0x[\da-fA-F]+(un|lf|LF)\b/g
        },

(at least one hexadecimal char and mandatory suffix)

        {
            pattern: /\b-?0b[01]+(y|uy)\b/g
        },

(at least one binary char and mandatory suffix)

        {
            pattern: /\b-?(\d+\.|\d*\.?\d+)([fFmM]|[eE][+-]?\d+)?\b/g
        },

(a number followed by a dot alone or a decimal number, followed by optional suffix or optional exponential notation)

        {
            pattern: /\b-?\d+(y|uy|s|us|l|u|ul|L|UL|I)?\b/g
        }
    ]

(an integer followed by optional suffix)

Do you think this handles all the cases?

@simon-reynolds
Copy link
Contributor Author

The patterns you suggest look good, better than what I could write
The only changes I made where to make the suffixes for binary and hexadecimal numbers optional as they are valid ints

@Golmote
Copy link
Contributor

Golmote commented Feb 8, 2015

Great!

This looks good to me, @LeaVerou.

@LeaVerou
Copy link
Member

Why are there objects with just a pattern attribute instead of just regexes? The object syntax is for when you need to set more attributes than just the pattern...

@simon-reynolds
Copy link
Contributor Author

Whoops, my bad. I've changed them to a simple array as per the documentation

@LeaVerou
Copy link
Member

LGTM, merging.
As a minor comment, the array would be more readable if every regex was on separate lines.

LeaVerou added a commit that referenced this pull request Feb 13, 2015
@LeaVerou LeaVerou merged commit e147458 into PrismJS:gh-pages Feb 13, 2015
@Golmote Golmote mentioned this pull request Feb 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants