Page MenuHomePhabricator

Switching to pygmentize -V
Needs ReviewPublic

Authored by tycho.tatitscheff on Oct 17 2015, 1:07 PM.
Tags
None
Referenced Files
F13990455: D14297.id34892.diff
Tue, Oct 22, 3:44 AM
F13979768: D14297.id34511.diff
Sat, Oct 19, 6:28 AM
F13975164: D14297.id34510.diff
Fri, Oct 18, 9:03 AM
F13974537: D14297.id34768.diff
Fri, Oct 18, 6:04 AM
Unknown Object (File)
Fri, Oct 11, 10:26 PM
Unknown Object (File)
Fri, Oct 11, 3:53 PM
Unknown Object (File)
Thu, Oct 10, 12:09 PM
Unknown Object (File)
Thu, Oct 10, 7:02 AM

Details

Summary

Fixes T9587.

It :

  • run pygmentize -V (instead of pygmentize -h ) which output a version sentence
    • if it fails, it will display the existing pygments.failed
    • it parses the sentence:
      • if the sentence is empty or don't match the regex /Pygments version (\d(\.\d)+)/, it will display a pygments.invalide
      • if the sentence match it will extract the version number:
        • if version isn't at least 2.0, it will display a pygments.outdated
        • else, it is fine 🌴 🍸

I also modularize the pht to extract pygmentize from the string and make message generalisable and thus translatable.

Test Plan

I basically tested quite all the steps (summary and message) to see I didn't break anything.

pygments.enabled

notfound_summary.png (58×1 px, 10 KB)

notworking_message.png (1×1 px, 106 KB)

pygments.failed

notworking_summary.png (57×1 px, 9 KB)

notworking_message.png (1×1 px, 106 KB)

Note that the url was ../pygments.invalide in the screenshot. It was corrected in the last diff !

pygments.invalide

invalid_summary.png (60×1 px, 11 KB)

invalid_message.png (1×1 px, 98 KB)

pygments.outdated

outdated_summary.png (61×1 px, 12 KB)

outdated_message.png (1×1 px, 98 KB)

Diff Detail

Repository
rP Phabricator
Branch
fix/pygments-should-advice-to-update
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8701
Build 10099: arc lint + arc unit

Event Timeline

tycho.tatitscheff retitled this revision from to [WIP] Switching to pygmentize -V.
tycho.tatitscheff updated this object.
tycho.tatitscheff edited the test plan for this revision. (Show Details)

@epriestley : something like this would be correct ?
Is there unit test I must write for this ?

cspeckmim added inline comments.
src/applications/config/check/PhabricatorPygmentSetupCheck.php
64

I don't think this will work. Check what the output of pygmentize -V is in order to be parsed; @epriestley gives examples from T9587#140655:

$ pygmentize -V
Pygments version 2.0.1, (c) 2006-2014 by Georg Brandl.

The version_compare will only be able to parse the 2.0.1 and not any of the rest of the line:
http://php.net/manual/en/function.version-compare.php

You will need to parse the version number out of $stdout prior to using it in version_compare in order for this to be a valid comparison.

67–68

This should probably list the existing version of pygmentize found, and the second sentence here might read better if phrased a little differently:

Upgrading pygmentize to version 2.0 or greater can improve performance in syntax highlighting.
73

I think these upgrade installations are too specific for a setup. Particularly in my case I do not have pip installed. The instructions on /config/edit/pygments.enabled/ indicate:

To install Pygments, visit pygments.org and follow the download and install instructions.

Deferring to installation instructions which pygments itself instructs. The upgrade instructions should probably be similar.

79

Does this need to reference something or can a new issue identifier be created on-the-spot?

84

I'm not very familiar with reporting issues in Phabricator - I'm guessing this line adds Phabricator's environment path to the output,and probably useful to display for this purpose.

@cspeckmim thank you from the multiple advices. I will correct this soon(TM).

epriestley added a reviewer: epriestley.

See @cspeckmim's feedback.

This revision now requires changes to proceed.Nov 2 2015, 4:41 PM

I will push my work later this week.

tycho.tatitscheff edited edge metadata.
  • Taking comment into account : not actually yet tested (i intend to switch python version and see all works as axpected)
tycho.tatitscheff added inline comments.
src/applications/config/check/PhabricatorPygmentSetupCheck.php
63–115

That was also my thought.

79

Sems so : I grep for pygments.failed and only found one entry.

src/applications/config/check/PhabricatorPygmentSetupCheck.php
40

I'm not sure what would work best here. Off-hand I think this would fail if one of the version numbers was more than 1 digit:

version 2.0.19

As this regex is looking for single digits. Here's some psuedo-php/regex

// match the output looking for just "version x.x.x", capturing "x.x.x" in the match group
preg_match('/version ([\d|\.]+)/', $stdout, $match);
// strip out the "version ", should do error checking in case nothing matched!
$match[0] = $match[0].substring("version ".length);

That's not correct php but I'm not very familiar with string processing in php and don't have time at the moment to look up the proper way to do it.

67

Needs a space -> "available. Upgrading"

src/applications/config/check/PhabricatorPygmentSetupCheck.php
40

or (\d+\.\d+\.\d+)

src/applications/config/check/PhabricatorPygmentSetupCheck.php
40

Yea, that's what I was thinking but would need to test. I didn't know if the + would work inside a capture group (). Maybe PHP's version_compare has available a regex it uses internally that we can utilize?

This comment has been deleted.
src/applications/config/check/PhabricatorPygmentSetupCheck.php
40
tycho.tatitscheff edited edge metadata.
  • Fixing stuff
  • Fixing Lint and adding original version number
tycho.tatitscheff retitled this revision from [WIP] Switching to pygmentize -V to Switching to pygmentize -V.Nov 8 2015, 12:13 PM
tycho.tatitscheff updated this object.
tycho.tatitscheff edited the test plan for this revision. (Show Details)
tycho.tatitscheff added inline comments.
src/applications/config/check/PhabricatorPygmentSetupCheck.php
31

@epriestley : can I rename this to 'pygments.notfound' ??

85

Maybe I should not hardcode 2.0.0 and make a variable like $min_perf_version ?

This style issue apart what version should we wait to get @gd improvements in ?

src/applications/config/check/PhabricatorPygmentSetupCheck.php
85

I think it would be 2.1 !

Possibly we should wait until that makes it to a release and specifically recommend the release containing that fix ("Upgrade to 2.0.5 or newer...", or whatever the next release number is) since it's probably the highest-impact user-facing change between 1.4 and today for most users, albeit only for a subset of inputs.

Shall I put on hold till 2.1 appears ?

src/applications/config/check/PhabricatorPygmentSetupCheck.php
45

TODO : Shall be pygments

@epriestley : pygments is now 2.1.1 so the diff will actually make user upgrade to a version that fix T9587.

However it has been a long time since I worked on that. If it still interest you, I can rebase on master, retest and make changes according to some code review. Else, lets abandon this.