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

Syntax highlighting breaks if the code starts with a partial comment #214

Closed
denis-sokolov opened this issue May 27, 2014 · 19 comments
Closed

Comments

@denis-sokolov
Copy link
Collaborator

1

We should be able to detect such situations and workaround them.

@staabm
Copy link
Contributor

staabm commented May 27, 2014

Whats your idea to work arround it? Just showing more lines of code?

@denis-sokolov
Copy link
Collaborator Author

Nah, that won't ever be guaranteed.
I don't know yet, I haven't thought too much.
But detecting that there's a */ sooner than /* in the code and injecting a hidden /* in there should cover most of the cases right off the bat, I think.

@filp
Copy link
Owner

filp commented May 27, 2014

@denis-sokolov That's the solution I tried once and it was, as far as I remember, consistently reliable -- memory fails me as to why I ended up not moving forward with it, unfortunately. It ended up looking something like:

/** (comment start added by whoops)
    * @return bool Some result
    */
    public function theFooing($muchFoo = true)
    {

Though indentation can also be corrected if that's deemed necessary.

@staabm
Copy link
Contributor

staabm commented May 27, 2014

Sounds great

@denis-sokolov
Copy link
Collaborator Author

Great, thanks for your input, @filp.

@staabm
Copy link
Contributor

staabm commented May 28, 2014

A fix for this should also take care that #205 won't get unfixable.

@filp
Copy link
Owner

filp commented May 28, 2014

@staabm Given the fairly simple process for fixing this issue, maybe it can be done in JS and applied to the source file at the time syntax highlighting is applied. Would also mean this transformation would only need to be done on parts of the code that the user is actually look at (I don't have the numbers to back it up, but I assume in the majority of cases users don't actually navigate to frames other than the first).

@denis-sokolov
Copy link
Collaborator Author

I navigate frames a lot. Not that it will hinder a fix significantly.

@ConnorVG
Copy link

What if the missing comment start was the reason the code was actually failing? Then you're just hiding the error.

@denis-sokolov
Copy link
Collaborator Author

That is a good observation, @ConnorVG!
We also need to confirm that there is an opening /* somewhere before the snippet.

@Anahkiasen
Copy link
Contributor

If the error is the comment, the beginning of the snippet won't be the comment itself since it takes a few lines before, no ?

@ConnorVG
Copy link

@Anahkiasen not really... Missing } sort of deal?

@GRMule
Copy link

GRMule commented May 21, 2015

I had an idea that I thought might work. After $range = $frame->getFileLines($line - 8, 10);, check the first line, and if the first non-space char is an asterisk, then get a new range including one more line above and check again. I figured you could do this a set number of times, then give up so as to not make the code sample too long in the name of chasing the comment head. Or, as ConnorVG pointed out, if the error IS the comment head then you'll never finish, so there would have to be a threshold for giving up.

This is my initial attempt at that idea:

$firstLine = trim(reset($range));
if ($firstLine) {
    $firstCharIsAsterisk = $firstLine[0] === '*';
    $giveup = 10; // give up after 10 tries to find the comment head
    $tryCtr = 0;
    while ($firstCharIsAsterisk !== false && $tryCtr < $giveup) {
        $range = $frame->getFileLines($line - 8 - $tryCtr, 10 + $tryCtr);
        $firstLine = trim(reset($range));
        $firstCharIsAsterisk = $firstLine[0] === '*';
        $tryCtr++;
    }
}

The problem is that intermediary lines of multiline comments don't have to start with an asterisk. My idea worked with this comment:

            /*
             * comment 1
             * comment 2
             * comment 3
             * comment 4
             * comment 5
             * comment 6
             * comment 7
             */
            $t = 1/0; // exception here

I got a larger code sample window that included the error line all the way up to the comment head. You could also just use that code to verify that there IS a comment head somewhere in there, then go with the original range but append the opening comment as filp mentioned.

This comment, however, broke my idea:

            /*
              comment 1
              comment 2
              comment 3
              comment 4
              comment 5
              comment 6
             */ 
            $t = 1/0; // exception here

Most people use the asterisk all the way down, but it isn't necessary. Without that as a hint, there's really no way I can think of to evaluate whether a given line, in isolation, is a comment or not.

Thought I would share my efforts in case this helps someone else think of a solution, or at least see what does not work. There may be a variation of this that does work, for example, you could start at the bottom of the range and look for the comment tail (*/), then you know that there ought to be a head somewhere above it.

@denis-sokolov
Copy link
Collaborator Author

I'd say we need to match on ^(/[^*]|[^/])+\*/ to know if we're in an opened multi-line comment situation,
then try to backtrack up to 10-ish lines to find /*, and if not found, prepend our own /* line-generated-by-Whoops.

@denis-sokolov
Copy link
Collaborator Author

Closing due to inactivity. If anyone wants to work on this, feel free to speak up.

@james-radford-854
Copy link

james-radford-854 commented Jul 28, 2016

It's not just comments that we need to look for:

print('Strings can
span multiple lines.
The following line:
/*
has no effect
because comments
cannot be started
inside strings.');

To have any real reliability we need to deal with the code from the beginning of the file to the end of the code we want to display.

We could do one of the following with the said code:

  1. Tokenise it server-side and output the snippet of interest as decorated markup – with the code inside <span>s with the correct scope classes.
  2. Tokenise it server-side and determine which scope (if any) is unterminated at the end of line before the first line of the snippet, and will consequently continue onto the first line of the snippet. Pass this beginning scope to JavaScript which will perform client-side syntax highlighting on the snippet, beginning on the scope specified by the server-side code.
  3. Supply the code to the browser and perform client-side syntax highlighting, removing the superfluous leading lines of context after we have determined the beginning scope of the snippet.

If option 1 or 2, does anyone have any recommendation of PHP-based server-side syntax highlighters?

If option 2 and we want to worry about the server-side and client-side syntax highlighters behaving exactly the same, perhaps we could convert prettify.js to PHP? 😅

@ConnorVG
Copy link

ConnorVG commented Aug 5, 2016

We could be naughty: syntax highlight the file then extract the portion we need.

rubs hands together manically

@acidjazz
Copy link
Contributor

acidjazz commented Aug 7, 2016

@filp @denis-sokolov how about #436 ?

@filips123
Copy link

@acidjazz @denis-sokolov This still sometimes not work. See #588 and googlearchive/code-prettify#553 for details.

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

No branches or pull requests

9 participants