1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-21 22:32:41 +01:00
Commit graph

8 commits

Author SHA1 Message Date
epriestley
3ded5d3e8c Disable the JSHint "function called before it is defined" and "unused parameter" warnings
Summary:
Ref T12822. The next change hits these warnings but I think neither is a net positive.

The "function called before it is defined" error alerts on this kind of thing:

```
function a() {
  b();
}

function b() {
}

a();
```

Here, `b()` is called before it is defined. This code, as written, is completely safe. Although it's possible that this kind of construct may be unsafe, I think the number of programs where there's unsafe behavior here AND the whole thing doesn't immediately break when you run it at all is very very small.

Complying with this warning is sometimes impossible -- at least without cheating/restructuring/abuse -- for example, if you have two functions which are mutually recursive.

Although compliance is usually possible, it forces you to define all your small utility functions at the top of a behavior. This isn't always the most logical or comprehensible order.

I think we also have some older code which did `var a = function() { ... }` to escape this, which I think is just silly/confusing.

Bascially, this is almost always a false positive and I think it makes the code worse more often than it makes it better.

---

The "unused function parameter" error warns about this:

```
function onevent(e) {
  do_something();
```

We aren't using `e`, so this warning is correct. However, when the function is a callback (as here), I think it's generally good hygiene to include the callback parameters in the signature (`onresponse(response)`, `onevent(event)`, etc), even if you aren't using any/all of them. This is a useful hint to future editors that the function is a callback.

Although this //can// catch mistakes, I think this is also a situation where the number of cases where it catches a mistake and even the most cursory execution of the code doesn't catch the mistake is vanishingly small.

Test Plan: Egregiously violated both rules in the next diff. Before change: complaints. After change: no complaints.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12822

Differential Revision: https://secure.phabricator.com/D20190
2019-02-19 15:13:56 -08:00
epriestley
f5c686d6a4 Swap charts from gRaphael to D3
Summary:
Mostly, this has just been sitting in my sandbox for a long time. I may also touch some charting stuff with subprojects/milestones, but don't have particular plans to do that.

D3 seems a bit more flexible, and it's easier to push more of the style logic into CSS so you can fix my design atrocities. gRaphael also hasn't been updated in ~3+ years.

Test Plan:
{F1085433}

{F1085434}

Reviewers: chad

Reviewed By: chad

Subscribers: cburroughs, yelirekim

Differential Revision: https://secure.phabricator.com/D15155
2016-02-01 10:36:59 -08:00
Joshua Spence
8113e6d8c1 Enable the freeze option for JSHint
Summary:
This option prohibits overwriting prototypes of native objects such as `Array`, `Date` and so on.

```lang=js
// jshint freeze:true
Array.prototype.count = function (value) { return 4; };
// -> Warning: Extending prototype of native object: 'Array'.
```

Test Plan: Linted existing JavaScript files, found no violations.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11439
2015-01-20 21:25:12 +11:00
Joshua Spence
53834d1471 Enable "strict" mode for NodeJS
Summary:
In particular, this changes the behavior of NodeJS in the following ways:

- Any attempt to get or modify the global object will result in an error.
- `null` values of `this` will no longer be evaluated to the global object and primitive values of this will not be converted to wrapper objects.
- Writing or deleting properties which have there writeable or configurable attributes set to false will now throw an error instead of failing silently.
- Adding a property to an object whose extensible attribute is false will also throw an error now.
- A functions arguments are not writeable so attempting to change them will now throw an error `arguments = [...]`.
- `with(){}` statements are gone.
- Use of `eval` is effectively banned.
- `eval` and `arguments` are not allowed as variable or function identifiers in any scope.
- The identifiers `implements`, `interface`, `let`, `package`, `private`, `protected`, `public`, `static` and `yield` are all now reserved for future use (roll on ES6).

Test Plan: Verified that Aphlict was still functional.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11430
2015-01-20 07:43:10 +11:00
Joshua Spence
af86a35f6b Allow JX to be redefined for NodeJS files
Summary: This silences a bunch of JSHint warnings.

Test Plan: `arc lint -- support/aphlict/server/lib/AphlictLog.js`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11084
2014-12-31 08:39:25 +11:00
Joshua Spence
b6e0c76c7d Define a seperate JSHint configuration for NodeJS files
Summary: Currently, we assume that all JavaScript files are for use in a browser. This is not true for the NodeJS Aphlict server code. Split the current JSHint configuration into `jshint-browser` and `jshint-node`.

Test Plan: `arc lint`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11071
2014-12-30 03:02:13 -08:00
Joshua Spence
db56342615 Declare Raphael as a global for JSHint
Summary: Let JSHint know that `Raphael` is a global that can be used anywhere. This is not technically correct, but it silences a few JSHint warnings. See http://jshint.com/docs/ for more information.

Test Plan: `arc lint -- webroot/rsrc/js/application/maniphest/behavior-line-chart.js`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11069
2014-12-30 02:58:22 -08:00
Joshua Spence
13590b9e3d Rename the support/jshint directory.
Summary: It is somewhat overkill to use an entire directory for a single file (the configuration file for JSHint). Instead, rename the directory so that it could (theoretically) be used for other linter configuration files.

Test Plan: Ran `arc lint -- webroot/rsrc/js/core/phtize.js` just to make sure everything still works.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9944
2014-07-16 22:07:55 +10:00