Update GitHub style

[Hirse]

Resolves #1616

As mentioned in the issue above, the GitHub-theme has not been updated since 2015 and is quite out of date.
The GitHub Gist theme has been updated and (since GitHub and Gist now share the same) looks closer to GitHub's current design, but not entirely.

The screenshots below show the old and new GitHub theme. There are some minor differences as GitHub uses more granularity for some cases (splitting number and unit in CSS) and different classifications across languages for tokens than HLJS.

Old GitHub theme

New GitHub theme

GitHub (for comparison)

Changes

Checklist

• Added markup tests N/A, theme only change
• Updated the changelog at CHANGES.md
• Added myself to AUTHORS.txt, under Contributors

[joshgoebel]

Awesome!

Please show a code comparison as well, say a medium snippet of Javascript, etc.

[Hirse]

@joshgoebel see the JS code below

New GitHub theme

GitHub (for comparison)

[joshgoebel]

What is the orange coloring of GitHub for Object? We consider Object a built_in, how is GitHub categorizing it?... is there a reason we didn't consider applying it?

[Hirse]

GitHub seems to consider all Capitalized functions (and variables) different to other functions.
See function MyObject vs function deepFreeze below.
As far as I can tell, Object is simply a class, but not a built-in Object is detected as a class, not a built-in.

I update my style a bit anyway to reflect some distinction between class and function where possible:

[joshgoebel]

GitHub seems to consider all Capitalized functions (and variables) different to other functions.
See function MyObject vs function deepFreeze below.

Ah, I think that's dating back to before we had classes... since a function can of course be a class/constructor type function even before classes existed as a thing.

As far as I can tell, Object is simply a class, but not a built-in.

Well, it's a JS built-in of course (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object), but if you mean that perhaps GitHub isn't treating it differently because of that you may be right.

Ok, now that we have more context I'm pretty confident that built_in and and title should not be the same stye... built_in just includes far too many things in JS.... see ecmascript.js... perhaps we should move some of the built_ins from built_in to type? built_in vs type has often been an annoying line. I don't love how we define them now with user defined type being different from system types - because most highlighters I've seen draw no distinction.

@allejo @egor-rogov Any thoughts here? Perhaps move all ecmascript TYPES into keywords: {type} and then I'd feel a little better about highlighting type/title the same... Or we're welcome to come up with a subcategory here if this makes sense for the first... built_in.class, type.built_in...(although if the subtype is just named the same as another class this is perhaps even more confusing). I do feel there are many languages where we should think of and highlight built-in functions vs built-in classes differently...

So the question is... is that what we have type for, or do we need something new?

@Hirse Your'e welcome to weigh in also of course. :)

[Hirse]

With the current logic, for the GitHub style it might be best to stick to purple for all titles (function and class) to be consistent.
Using the orange for built-in feels reasonable too.

Would that be a reasonable solution while the bigger changes to JS grammar/logic are being worked out?

[joshgoebel]

Playing:

// js
interface User {
  name: string;
  id: number;
}

bob.User.Smithy.conquest()

class UserAccount {
  name: string;
  id: number;

  constructor(name: string, id: number) {
    this.name = name;
    this.id = id;
  }
}

const user: User = new UserAccount("Murphy", 1);

// ts
interface User {
  name: string;
  id: number;
}

class UserAccount {
  name: string;
  id: number;

  constructor(name: string, id: number) {
    this.name = name;
    this.id = id;
  }
}

const user: User = new UserAccount("Murphy", 1);

let Bug = 53

  as, // for exports
  in,
  of,
  if,
  for,
  while,
  finally,
  var,
  new,
  function,
  do,
  return,
  void,
  else,
  break,
  catch,
  instanceof,
  with,
  throw,
  case,
  default,
  try,
  switch,
  continue,
  typeof,
  delete,
  let,
  yield,
  const,
  class,
  // JS handles these with a special rule
  // get,
  // set,
  debugger,
  async,
  await,
  static,
  import,
  from,
  export,
  extends
];
const LITERALS = [
  true,
  false,
  null,
  undefined,
  NaN,
  Infinity
];

const TYPES = [
  Intl,
  DataView,
  Number,
  Math,
  Date,
  String,
  RegExp,
  Object,
  Function,
  Boolean,
  Error,
  Symbol,
  Set,
  Map,
  WeakSet,
  WeakMap,
  Proxy,
  Reflect,
  JSON,
  Promise,
  Float64Array,
  Int16Array,
  Int32Array,
  Int8Array,
  Uint16Array,
  Uint32Array,
  Float32Array,
  Array,
  Uint8Array,
  Uint8ClampedArray,
  ArrayBuffer
];

const ERROR_TYPES = [
  EvalError,
  InternalError,
  RangeError,
  ReferenceError,
  SyntaxError,
  TypeError,
  URIError
];

const BUILT_IN_GLOBALS = [
  setInterval,
  setTimeout,
  clearInterval,
  clearTimeout,

  require,
  exports,

  eval,
  isFinite,
  isNaN,
  parseFloat,
  parseInt,
  decodeURI,
  decodeURIComponent,
  encodeURI,
  encodeURIComponent,
  escape,
  unescape
];

const BUILT_IN_VARIABLES = [
  arguments,
  this,
  super,
  console,
  window,
  document,
  localStorage,
  module,
  global // Node.js
];

[allejo]

Ok, now that we have more context I'm pretty confident that built_in and and title should not be the same stye...

Agreed 👍

@allejo @egor-rogov Any thoughts here? Perhaps move all ecmascript TYPES into keywords: {type} and then I'd feel a little better about highlighting type/title the same... Or we're welcome to come up with a subcategory here if this makes sense for the first... built_in.class, type.built_in...(although if the subtype is just named the same as another class this is perhaps even more confusing). I do feel there are many languages where we should think of and highlight built-in functions vs built-in classes differently...

So the question is... is that what we have type for, or do we need something new?

If I'm understanding this and RTD correctly, type is used for user-defined types. I don't think there's a need to style type differently from built_in. If we want to differentiate between built-in types and user-defined types, then yes they should be styled differently and add some type of subcategory as you proposed. What possible conflicts are there right now with class names?

[joshgoebel]

If I'm understanding this and RTD correctly, type is used for user-defined types.

I think I'm going to change that definition in the docs, we already regularly use it more broadly. For example in some languages we do:

• type - builtin types
• built_in - builtin functions

This is honestly how I think of them in my head and personally I'm not sure type.user-defined vs type.builtin is that useufl of a distinction in most cases. See the new sql grammar for example... varchar2 being a type but avg(price) being a use of the built_in avg.

I mean technically these could be builtin.func and builtin.type, but sadly we're not there yet - though I think that's a taste of where we're headed. The other complexity here is that many languages don't draw a big distinction between built-in vs user "types"... we've already been talking about adding things like title.class and title.function... so in this case is Array a built_in, a type, or a title.class?

I think I'd be pretty happy to add a CamelCase => title.class rule for several a few languages and avoid the need for a list of built-ins at all...

[joshgoebel]

With the current logic, for the GitHub style it might be best to stick to purple for all titles (function and class) to be consistent.

I like this idea the least. There is a distinction between function and class titles that we should try to preserve... so function title should be purple and title alone can stay orange...

The only question is what to do about built-in and built-in vs type.

[joshgoebel]

I think I'm "ok" with leaving built-in as orange for now and revisiting this at a later date...

[joshgoebel]

Did we lose .hljs-subst?

[joshgoebel]

Sorry just now taking a CLOSE look at this. :-) Before I was mostly just looking at the pictures.

.hljs-comment and .hljs-meta should not be the same. I'd think the meta should probably be the pretty light blue color (which would match the DOCTYPE snap to a tee).

[allejo]

I think I'm going to change that definition in the docs, we already regularly use it more broadly. For example in some languages we do:

• type - builtin types
• built_in - builtin functions

+1

This is honestly how I think of them in my head and personally I'm not sure type.user-defined vs type.builtin is that useufl of a distinction in most cases. See the new sql grammar for example... varchar2 being a type but avg(price) being a use of the built_in avg.

As you've mentioned to me before, highlight.js' goal is to get users most of the way there when it comes to syntax highlighting. I think being able to differentiate between built-in and user-defined types goes against that goal and is introducing an unnecessary level of maintenance (maintaining a list of built-ins per language).

I think I'd be pretty happy to add a CamelCase => title.class rule for several a few languages and avoid the need for a list of built-ins at all...

I like this idea, seems like the least specific approach and will benefit users for the majority of the cases.

[joshgoebel]

being able to differentiate between built-in and user-defined types goes against that goal and is introducing an unnecessary level of maintenance (maintaining a list of built-ins per language).

I'm always forgetting about auto-detect though, that's what maintaining the built-in lists does for ya... but still for languages where it's easy to detect proper class names by case I think I'd be a fan of an additional rule so we can color them all. And at that point the highlighting distinction (color) may not matter, but internally we'd still know what is what is what for relevance.

I do think overtime that makes it harder to maintain the built-in class lists (since now no one can see when they are deficient), but that's perhaps a problem for another day.

[allejo]

And at that point the highlighting distinction (color) may not matter, but internally we'd still know what is what is what for relevance.

Using built-in lists internally for relevance definitely makes sense. I don't think there's much use for it as a CSS class though, so I'd opt out of including that distinction as a class; one less thing to worry about people potentially relying on.

[joshgoebel]

@Hirse I see some questions we've asked here you've never responded to... can we bring this back to life or should I just close it for now?

[Hirse]

Hey @joshgoebel, sorry I completely forgot about this PR.

I'm going to have a look to read the conversation (again) and address the questions.

[Hirse]

.hljs-comment and .hljs-meta should not be the same. I'd think the meta should probably be the pretty light blue color (which would match the DOCTYPE snap to a tee).

GitHub's highlighting doesn't seem to have the same concept or meta

Java: All annotations as keyword (notice punctuation is also styled as keyword)

@RunWith(SpringRunner.class)
@Something(1+(3+4))
public class BoardControllerTest {
}

HTML: doctype annotation as constant

<!doctype html>
<html>

C++: # without color, include as keyword, rest as string`

// Meta strings
#include <stdio>
#include "lib.h"

Setting meta to the light blue as suggested brings us quite close for HTML and C++, but rather different for Java:

[joshgoebel]

I'm thinking this was incorrectly flagged "good first issue" LOL. 😐

[joshgoebel]

I think the latest snaps look fine. I think perhaps we should rename this "GitHubish" though LOL.

[Hirse]

Did we lose .hljs-subst?

It's the same color as default.
Re-added to be correct when inside hljs-string.

[joshgoebel]

@allejo Any thoughts? I think for these themes I'll settle for continuous improvements vs postponed perfection.

@Hirse Did you look into adding punctuation and operator which we now support? (I didn't check)

[joshgoebel]

And is this technically a breaking change? 🤔

[Hirse]

And is this technically a breaking change? 🤔

It might be since it's a rather substantial change.
There is no urgent bug being fixed so I'm fine having it land in v11 to be safe.

[joshgoebel]

There is no urgent bug being fixed so I'm fine having it land in v11 to be safe.

Yeah, we'll merge it after 10.7.

[joshgoebel]

Are we considering this good to go now then?

[Hirse]

Yes, I would think so.

[allejo]

@allejo Any thoughts? I think for these themes I'll settle for continuous improvements vs postponed perfection.

👍 for continuous improvements. There's no way we'd be able to get perfection (even if postponed) here without a lot of effort.

GitHub.com

This PR

GitHub.com C#

This PR

This inaccuracy is partly because of the C# grammar (i.e. the ///) but also, these are both correctly highlighted as doctags. However, it appears that GitHub has some special highlighting for C# doctags (something hljs doesn't support).

And is this technically a breaking change? 🤔

Enough of a breaking change to warrant a merge for 10.7.

[joshgoebel]

Enough of a breaking change to warrant a merge for 10.7.

Since it looks so different I figure it doesn't hurt to just save it for 11. I want to push 10.7 soon, so it won't wait too long to get merged.