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

enh(php) Add additional keywords, built-in classes, and '<?=' syntax #2372

Merged
merged 16 commits into from Feb 9, 2020

Conversation

taufik-nurrohman
Copy link
Member

@taufik-nurrohman taufik-nurrohman commented Jan 31, 2020

PS: For maintainers, you may need to unify attr class name to attribute or simply include .hljs-attr along with .hljs-attribute class declaration.

https://highlightjs.readthedocs.io/en/latest/css-classes-reference.html (#issuecomment-580729326)

+ `__TRAIT__`
+ `bool`, `callable`, `float`, `from`, `int`, `iterable`, `object`, `string`, `void` <https://www.php.net/manual/en/language.types.intro.php> <https://www.php.net/manual/en/migration71.new-features.php>
+ `fn` <https://wiki.php.net/rfc/arrow_functions_v2>
+ `true`, `false` and `null` are now literals.
@joshgoebel
Copy link
Member

Please don't mix a bunch of items in a single PR, that makes it much harder to review and will slow things down.

@joshgoebel
Copy link
Member

joshgoebel commented Jan 31, 2020

I mean grouping the PHP things is ok, but the other stuff should preferably be split out... typically we squash commits here, which is another reason for this - since most people are bad/terrible with maintaining a proper commit history otherwise. It looks like you MIGHT be an exception in that regard (good commit name, splits) but still it makes things more complex to review vs 3 smaller PRs.

For example if this had just been PHP stuff it likely would already be merged, without any need to get into the complexities of attr vs attribute, etc.

@joshgoebel
Copy link
Member

PS: For maintainers, you may need to unify attr class name to attribute or simply include .hljs-attr along with .hljs-attribute class declaration.

These are very different things on purpose. You can search issues in the past to find the write up, sorry I don't have it off the top of my head. So you'd have to review other themes to see if attr aliasing to attribute is something we would accept. Some themes purposely only highlight one and choose to ignore the other.

@joshgoebel
Copy link
Member

I will approve and flat-merge if you simply pull out 622f5cb. We can discuss that elsewhere.

@joshgoebel
Copy link
Member

OK but now you're going to have to look at the PHP/zephir collision. It may be that you need to apply your PHP updates to Zephir as well.

Sorry just now noticed the failing test.

@joshgoebel
Copy link
Member

Probably wouldn't hurt to add a comment to the PHP and Zephier files to tell maintainers to cross-reference them when making updates.

I also wouldn't mind if Zephir pulled in PHP as a requirement and built upon it (which would avoid these issues in the future) but that might be a bit more work than you were wanting to put it. Let me know. :)

@joshgoebel
Copy link
Member

--- src/languages/php.js	2020-01-31 08:28:57.000000000 -0500
+++ src/languages/zephir.js	2020-01-31 08:28:54.000000000 -0500
@@ -1,21 +1,14 @@
 /*
-Language: PHP
-Author: Victor Karamzin <Victor.Karamzin@enterra-inc.com>
-Contributors: Evgeny Stepanischev <imbolk@gmail.com>, Ivan Sagalaev <maniac@softwaremaniacs.org>
-Website: https://www.php.net
-Category: common
-*/
+ Language: Zephir
+ Description: Zephir, an open source, high-level language designed to ease the creation and maintainability of extensions for PHP with a focus on type and memory safety.
+ Author: Oleg Efimov <efimovov@gmail.com>
+ Website: https://zephir-lang.com/en
+ */
 
 function(hljs) {
-  var VARIABLE = {
-    begin: '\\$+[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*'
-  };
-  var PREPROCESSOR = {
-    className: 'meta', begin: /<\?(php)?|\?>/
-  };
   var STRING = {
     className: 'string',
-    contains: [hljs.BACKSLASH_ESCAPE, PREPROCESSOR],
+    contains: [hljs.BACKSLASH_ESCAPE],
     variants: [
       {
         begin: 'b"', end: '"'
@@ -29,20 +22,21 @@
   };
   var NUMBER = {variants: [hljs.BINARY_NUMBER_MODE, hljs.C_NUMBER_MODE]};
   return {
-    aliases: ['php', 'php3', 'php4', 'php5', 'php6', 'php7'],
+    aliases: ['zep'],
     case_insensitive: true,
     keywords:
       'and include_once list abstract global private echo interface as static endswitch ' +
-      'array null if endwhile or const for endforeach self var while isset public ' +
+      'array null if endwhile or const for endforeach self var let while isset public ' +
       'protected exit foreach throw elseif include __FILE__ empty require_once do xor ' +
       'return parent clone use __CLASS__ __LINE__ else break print eval new ' +
       'catch __METHOD__ case exception default die require __FUNCTION__ ' +
       'enddeclare final try switch continue endfor endif declare unset true false ' +
       'trait goto instanceof insteadof __DIR__ __NAMESPACE__ ' +
-      'yield finally',
+      'yield finally int uint long ulong char uchar double float bool boolean string' +
+      'likely unlikely',
     contains: [
+      hljs.C_LINE_COMMENT_MODE,
       hljs.HASH_COMMENT_MODE,
-      hljs.COMMENT('//', '$', {contains: [PREPROCESSOR]}),
       hljs.COMMENT(
         '/\\*',
         '\\*/',
@@ -66,23 +60,9 @@
       ),
       {
         className: 'string',
-        begin: /<<<['"]?\w+['"]?$/, end: /^\w+;?$/,
-        contains: [
-          hljs.BACKSLASH_ESCAPE,
-          {
-            className: 'subst',
-            variants: [
-              {begin: /\$\w+/},
-              {begin: /\{\$/, end: /\}/}
-            ]
-          }
-        ]
-      },
-      PREPROCESSOR,
-      {
-        className: 'keyword', begin: /\$this\b/
+        begin: '<<<[\'"]?\\w+[\'"]?$', end: '^\\w+;',
+        contains: [hljs.BACKSLASH_ESCAPE]
       },
-      VARIABLE,
       {
         // swallow composed identifiers to avoid parsing them as keywords
         begin: /(::|->)+[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*/
@@ -98,7 +78,6 @@
             begin: '\\(', end: '\\)',
             contains: [
               'self',
-              VARIABLE,
               hljs.C_BLOCK_COMMENT_MODE,
               STRING,
               NUMBER

Ok I never looked closely before. I didn't realize they were so similar. I can probably consolidate them myself, so I wonder if there is a simpler way just to get your tests passing so we could get the PHP portions of this merged?

@taufik-nurrohman
Copy link
Member Author

Hmm. I’m confused. How can I revert specific item from a pull that has been requested? Can I just undo modifying the agate.css file with a new commit?

@joshgoebel
Copy link
Member

joshgoebel commented Jan 31, 2020

The preferable way would be to rebase onto master and just snip out the bad commit... if you want to try:

# make sure master branch is up-to-date!!!
git checkout [your branch]
git rebase -i master
# change the commit in question to removal
# save changes
# your commit history should be rewritten
# force push changes back to your branch/pull request
git push -f

Rewriting history in branches is one of gits amazing and powerful features.

@joshgoebel
Copy link
Member

Can I just undo modifying the agate.css file with a new commit?

That would typically have been ok since we usually squash the commit history (although you'd do it with revert not manually). But since your commits are so nice it'd be better here if you edited the history so we can preserve the commit history.

@taufik-nurrohman
Copy link
Member Author

Thanks for the instructions. It’s almost midnight here. I will try it tomorrow.

@joshgoebel
Copy link
Member

Once you see the power hopefully you'll go off and learn a bit more about rebasing, it's SUPER useful if you use git a lot and contribute to projects.

@taufik-nurrohman
Copy link
Member Author

Done with drop instead of removal 👍

Just realized that HTML attributes looks ugly (too striking) when highlighted. I don’t know, maybe this is very subjective.

Highlighted as .hljs-attribute Color

Screenshot from 2020-02-01 09 47 41

Not Highlighted

Screenshot from 2020-02-01 09 49 41

@joshgoebel
Copy link
Member

Just realized that HTML attributes looks ugly (too striking) when highlighted.

Well, that all depends on your theme. Some are awful, some are good IME.

@joshgoebel
Copy link
Member

joshgoebel commented Feb 1, 2020

This isn't truly rebased onto upstream master though or there wouldn't be any conflicts... you first need to make sure YOUR local master points to upstream/master. Where upstream = the highlightjs core repository.

I usually have upstream as a git remote so I can just fetch --all and then merge upstream/master.

Oh you really should be using branches for development not master. :-) Generally you should never commit to master (for projects you don't own), but rather use branches.

So you probably should:

  • Turn your work into a branch. (git checkout -b branch_name)
  • reset --hard your master to upstream/master
  • rebase again.

If that's all sounding too complex then you can also just remove the unrelated htaccess changes here and let this just be PHP changes (which we can squash). But this is why we usually prefer focused PRs. :-)

Working with larger ones is a pain UNLESS you're really familiar with Git. (not to mention being harder to review, etc)

@taufik-nurrohman
Copy link
Member Author

Most of the contributions I have made are because I use the application, not because I am proficient with Git. I will usually delete the fork repository when my pull requests has been merged, and will re-fork it when I want to make new pull requests 😛

@joshgoebel
Copy link
Member

I will usually delete the fork repository when my pull requests has been merged, and will re-fork it when I want to make new pull requests 😛

That works, but makes it hard to make multiple PRs against a single repo at once. :-)

@joshgoebel
Copy link
Member

joshgoebel commented Feb 1, 2020

It seems you added:

  • __TRAIT__,
  • 'bool',
  • 'callable',
  • 'float',
  • 'from',
  • 'int',
  • 'iterable',
  • 'object',
  • 'string',
  • 'void'

Possibly missing:

  • boolean
  • integer
  • double
  • ...

Where did you get this list of types? just checking quickly I see you've added bool, but not boolean... int, but not integer, etc...?

Also what is from, I'm not seeing any docs on it?


And removed:

  • 'exception',
  • 'interface',
  • 'use'

Please explain the removals. (also quite likely the reason the tests are failing)

We support back to PHP3 so I'm not sure anything should be removed;

@joshgoebel joshgoebel self-requested a review February 1, 2020 07:14
@joshgoebel
Copy link
Member

joshgoebel commented Feb 1, 2020

If that's the full list that's not so terrible... what is that? 50-80 items maybe? I'm fine with that IF you think they'd be useful for highlighting. If not lets just add Exception and be done. Your call. :)

 - This moves `exception`, `parent`, `self` and `static` keyword to built-in.
 - Added `class`, `extends`, `implements`, `interface` and `use` back to the keyword list.
 - Added type aliases such as `boolean` and `integer` (alias of `bool` and `int`)  to the keyword list.
 - Finally found a way to highlight keywords within function arguments based on `csharp.js` language highlighter.
@taufik-nurrohman
Copy link
Member Author

Trying to remove Sync and Update log but could’t find it when trying to rebase.

@taufik-nurrohman
Copy link
Member Author

Before After
before after
function foo(bool $a): bool {
  return $a;
}

$b = (bool) foo(false);


$b = (bool) $b;
$b = (boolean) $b;


function foo(boolean $a, integer $b = 1, string $c = 'wow') {}

foo(false, 1);


function foo(Boolean $a, Integer $b) {}

// TODO: Mark built-in interface/class name declaration as `built_in`
class Foo extends Bar implements ArrayAccess {
    public function bar(Boolean $a, Integer $b, Traversable $c) {
        $a = self::class;
        $b = static::class;
        $c = parent::class;
    }
}

$foo = new Foo;

$foo->bar(int $a = 1);


// Class import
use Author\Package\ClassName1;
use Author\Package\ClassName2;

// Variable import
$foo = 1;
$baz = function($qux) use($foo) { // TODO: Mark `use` as keyword here
    return $foo + $qux;
};


try {
    // ...
} catch (Exception $e) {
    // ...
}


class MyException extends Exception {}

try {
    // ...
} catch (MyException $e) {
    // ...
}

@joshgoebel
Copy link
Member

Trying to remove Sync and Update log but could’t find it when trying to rebase.

Does't matter, we'll squash this now... but what you probably want is to mark them as "fixup" then they'll be silently merged into the prior commit. In order to rebase properly you really want your master to point to a clean state. If you're rebasing onto yourself (which I think you're doing) I'm not sure how that works... I always branch, so I'm rebasing my branches on top of master.

@joshgoebel
Copy link
Member

Now your'e conflicting with Java... we may need to improve our Java sample.

@joshgoebel joshgoebel changed the title Added More Keywords for PHP enh(php) Add additional keywords and built-in classes Feb 2, 2020
@joshgoebel
Copy link
Member

joshgoebel commented Feb 2, 2020

This may have to wait now until after #2387 gets merged. (rather than keep fixing Zephir issues here)

@egor-rogov
Copy link
Collaborator

Oh, I see you guys have done a great job here and in #2387. What do you think about adding some tests? We have not a single one for Zephir yet...

@joshgoebel
Copy link
Member

I don't tend to require tests for contributors that are simply adding keywords. That's really a feature of HLJS, not something we need a test fore (when the keywords are just global, like here).

We have not a single one for Zephir yet...

I was using the detect file for all my testing... shall I just copy it wholesale over into the markup folder with it's expected result?

@egor-rogov
Copy link
Collaborator

That's right, my concern was more about #2387.

I was using the detect file for all my testing... shall I just copy it wholesale over into the markup folder with it's expected result?

It is better than nothing, and will make it easier to add other tests afterwards.

@joshgoebel
Copy link
Member

Ok, the only failing test now seems to be regarding how you've changed params... I actually raised my eyebrows when I saw how you changed it, but I need to look at all our other grammars and see how we're doing it there. Either the () are part of params or not, but we should be consistent about it.

Did you already look around before making this changes?

@taufik-nurrohman
Copy link
Member Author

taufik-nurrohman commented Feb 7, 2020

Did you already look around before making this changes?

I did.

Either the () are part of params or not, but we should be consistent about it.

I could make you become very very confusing in seconds without magic: with ES6 arrow functions :trollface:

const foo = (bar, baz) => {};
const bar = baz => {};

@joshgoebel
Copy link
Member

I did.

Ok, awesome. Now can you just fix the one failing test and then I think this might be ready to go!

@joshgoebel joshgoebel changed the title enh(php) Add additional keywords and built-in classes enh(php) Add additional keywords, built-in classes, and <?= syntax Feb 7, 2020
@joshgoebel joshgoebel changed the title enh(php) Add additional keywords, built-in classes, and <?= syntax enh(php) Add additional keywords, built-in classes, and '<?=' syntax Feb 7, 2020
@joshgoebel
Copy link
Member

I think you only fixed half of it. If you can run the tests yourself locally that'd make checking a lot easier.

@joshgoebel
Copy link
Member

@taufik-nurrohman Thanks for putting in the time on this one!

@joshgoebel joshgoebel merged commit 71a32c3 into highlightjs:master Feb 9, 2020
taufik-nurrohman added a commit to taufik-nurrohman/highlight.js that referenced this pull request Feb 18, 2020
…ighlightjs#2372)

* Added More Keywords

+ `__TRAIT__`
+ `bool`, `callable`, `float`, `from`, `int`, `iterable`, `object`, `string`, `void` <https://www.php.net/manual/en/language.types.intro.php> <https://www.php.net/manual/en/migration71.new-features.php>
+ `fn` <https://wiki.php.net/rfc/arrow_functions_v2>
+ `true`, `false` and `null` are now literals.

* `<?=` Tag is Enabled By Default Since PHP 5.4
* enh(zephir) add `fn` keyword for => functions
* chore(zephir) boost score by using likely/unlikely
* Added Built-In Keywords

 - This moves `exception`, `parent`, `self` and `static` keyword to built-in.
 - Added `class`, `extends`, `implements`, `interface` and `use` back to the keyword list.
 - Added aliases such as `boolean` and `integer` (alias of `bool` and `int`)  to the keywords.
 - Highlight keywords within function arguments (based on `csharp.js` language).

* fix auto-detect conflict with Java
* Exclude Parenthesis from `.hljs-params`

Co-authored-by: Josh Goebel <me@joshgoebel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants