Upgrade XO and add Prettier

[j-f1]

This upgrades XO to the latest version, fixing newly reported errors. It also adds Prettier, which standardizes formatting end helps ensure consistency.

Appending ?w=1 to the diff URL makes the changes more readable.

(full disclosure: I’m a Prettier contributor)

[makyen]

General

IMO, these are the type of changes which you should first open an issue and discuss, prior to submitting a PR. Discussing this with the various stakeholders first and obtaining buy-in from the various userscript projects prior to proposing code changes allows you to get buy-in that any change along these lines is even desired. After that, you can have a discussion and get agreement on the specifics of the formatting choices.

Basically, by issuing a PR first, you make it appear, or at least engender the feeling, that you are attempting to impose on the project your personal opinion of what code formatting should be. (OK, it's a view of "good" code formatting that you probably share with your "Prettier" project, but from the POV of this project, it's your own personal opinion of what code should look like.) Starting from this point will naturally set some people in opposition to the change, because of the way that you went about it, rather than discussing it and getting some agreement that change of this type is desirable.

While I'm generally in favor of having consistent code styles, it's been the choice of this project to permit each userscript to establish their own formatting and parameters for xo. Imposing additional formatting changes upon multiple scripts does not feel appropriate (even upon just those scripts that have not established their own xo parameters), without having first discussed this.

[makyen]

As to the specific code formatting changes chosen, I agree with some of them, but others I feel are detrimental to readability/maintainability.

Brief review of some specific changes

Breaking "long" lines in detrimental ways

Many of the changes you're making appear to be to satisfy some arbitrary length requirement, rather than focusing on readability/maintainability. For instance, You change:

log("AEKS cancelled: User has no account or AEKS already finished on this site.");

to:

  log(
    "AEKS cancelled: User has no account or AEKS already finished on this site."
  );

IMO, a change like this has no benefit, and reduces readability. Frankly, there are a large number of changes like this, few/none of the ones in this category make the code easier to read, and many/most make it harder. This appears to be due to imposing an automatic formatting requirement that lines won't exceed some (quite small) length limit. However, where these have been applied, they are mostly harmful, not helpful.

A line-length requirement opens up a few different issues, not the least of which is "how long is too long". Based on the change made to the above line, you've chosen something in the 80 character range. While that length may have made sense 30 years ago, displays these days are far more capable. While some length guidelines, intelligently applied, are, IMO, desirable, a fixed limit, particularly one so small, is not.

IMO, breaking lines like this is worse than useless. You've still ended up with a line that exceeds your set limit. You've just made it harder to read by spreading it over three lines. There are times when it's a really good idea to have a limit on the length of lines. However, this, specifically, is not the type of change that makes code better.

There are many examples of this in the changes you've made.

Multiple lines for arguments and parameters

While this can be beneficial, in some situations, in others it can, IMO, significantly reduce readability/maintainability. This may just be an extension of having set a low limit on the line length.
The lines below slightly exceed 80 characters. I haven't checked either the xo parameters you've set or all the instances of this type of change to determine if it's parameters specifically that are limited, or line-length.

Example:

You've changed the following three statements (these are simple to read and compare by just scanning down the three lines):

autoflagging.decorate.feedback.addFeedback(simpleFeedbacks.k, $feedback, "tpu-");
autoflagging.decorate.feedback.addFeedback(simpleFeedbacks.f, $feedback, "fp-");
autoflagging.decorate.feedback.addFeedback(simpleFeedbacks.n, $feedback, "naa-");

Into the following 15 lines, which require scanning the eye back and forth across multiple lines to determine that, in fact, they are almost completely identical:

autoflagging.decorate.feedback.addFeedback(
  simpleFeedbacks.k,
  $feedback,
  "tpu-"
);
autoflagging.decorate.feedback.addFeedback(
  simpleFeedbacks.f,
  $feedback,
  "fp-"
);
autoflagging.decorate.feedback.addFeedback(
  simpleFeedbacks.n,
  $feedback,
  "naa-"
);

At least to me, what you've changed this to is far less readable than what it was originally.

There are, again, many examples of this type of change in what you've done.

var to let or const

You appear to have changed many (all?) instances of var to let or const. Did you have xo do this automatically, or did you individually make each change and fully test each script/change? IME, xo does not correctly determine all of the dependencies and does make var -> let changes where making the change causes bugs/failures.

While I'm generally a proponent of using let/const, using them is not universally desirable, despite the fact that many people advocate for using only those.

function(foo) to foo =>

You have made many changes of function(foo) to foo =>. This appears to just be a syntax preference.

These methods of defining functions are not fully interchangeable.

Of most concern to me in this change is that you have done this in code that's using jQuery. jQuery commonly relies on the passed function being created with function() and often will fail to work when the function is created with () =>. Were these changes made by hand and tested, or were they automatically changed due to setting a preference in xo? Were they all tested, or at least individually examined? Note, again IME, performing an automatic change of this type can result in failures.

What's the point?

You've changed (simplified to highlight change):

const smiley = String.fromCodePoint(0x1F604);

to

const smiley = String.fromCodePoint(0x1f604);

I don't see the point of changing to use lowercase for hexadecimal numbers. Frankly, I find the uppercase version easier to read.

Far from a full review

This is far from a full review of these changes. Fully reviewing these changes would take quite a bit of time. It's not clear to me that we want to proceed with this at this time, so I'm not spending that time.

[ArtOfCode-]

TL;DR: if the individual scripts can choose the parameters for Prettier, I'm fine with this; if not, we need a discussion with script authors.

Upgrading xo is a good thing in either case.

[angussidney]

Related: #37

[j-f1]

@makyen

IMO, these are the type of changes which you should first open an issue and discuss, prior to submitting a PR.

I’m sorry if I came off as trying to impose a style. I meant for this to be a discussion, since it’s harder to debate whether or not to make these changes if you can’t actually see what’s going to be changed.

Breaking "long" lines in detrimental ways
Multiple lines for arguments and parameters

I agree that these don’t look great, but I feel like it’s one of the tradeoffs that you make so you never need to worry about formatting. Although we could increase the printWidth, Prettier’s docs recommend against doing so because it can harm readability by squishing multiple lines together where a human typically wouldn’t.

var to let or const

I allowed xo to make the changes automatically.

IME, xo does not correctly determine all of the dependencies and does make var -> let changes where making the change causes bugs/failures.

Yikes, do you have a reproducible example of this? If so, that’s a bug in ESLint that should be fixed.

function(foo) to foo =>

These syntaxes are almost fully interchangeable. The only differences AFAIK are that you can’t construct (new) an arrow function, that the this value stays the same as it was outside the function rather than being able to be customized by the caller, and that arguments is either inherited or unavailable. The autofixer didn’t change functions, that use this or arguments.

Of most concern to me in this change is that you have done this in code that's using jQuery. jQuery commonly relies on the passed function being created with function() and often will fail to work when the function is created with () =>.

If you have an example of jQuery throwing an error, that’s a bug which should be reported. Otherwise, the only case where arrow functions could cause problems is where you’re trying to usethis inside the function, which is covered by the condition I mentioned above.

What's the point [of lowercasing hexadecimals]?

This is one of those cases where Prettier picks a style and uses it. This makes code more consistent, rather than having some numbers be uppercase and others lowercase.

@ArtOfCode-

TL;DR: if the individual scripts can choose the parameters for Prettier, I'm fine with this; if not, we need a discussion with script authors.

You should be able to put a .prettierrc.yml inside the userscript folder to change Prettier’s options for that folder. Either way, a discussion with userscript authors is what I was intending. (That’s why I opened this PR rather than just pushing a commit)

---

That’s the best I can do at 6am :) If you want a longer explanation for something, please don’t hesitate to ask & I’ll see what I can do.

[j-f1]

Oh, one more thing: almost all of the syntax changes came from XO. Prettier only changes whitespace and a few other things, like parens and semicolons.