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

misc edits #13

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a4234e0
declare process, or I can't compile this
KiaraGrouwstra Jan 20, 2017
1680235
merge
KiaraGrouwstra Jan 22, 2017
efd20ae
add prepublish script, fixes missing checker.js
KiaraGrouwstra Jan 22, 2017
9bc9613
ditch original file.
KiaraGrouwstra Jan 22, 2017
1ae10d0
dry interfaces
KiaraGrouwstra Jan 22, 2017
77411da
use local tsconfig.json
KiaraGrouwstra Jan 22, 2017
ed96cc0
add option to hide line numbers for diff logs
KiaraGrouwstra Jan 22, 2017
be8019a
in the error message show the 'real' line numbers
KiaraGrouwstra Jan 22, 2017
8439093
improve attach failure log
KiaraGrouwstra Jan 22, 2017
3810735
fix superfluous colon
KiaraGrouwstra Jan 22, 2017
8894b3c
convert TS to lenient peer dependency.
KiaraGrouwstra Jan 22, 2017
c64000c
re-add TS in dev dependencies as well
KiaraGrouwstra Jan 22, 2017
2680172
error out on bad file paths
KiaraGrouwstra Jan 22, 2017
94c0cf2
handle edge case of diagnostics with undefined file.
KiaraGrouwstra Jan 22, 2017
8f20488
ditch superfluous logging statement
KiaraGrouwstra Jan 22, 2017
7016983
externalize interfaces
KiaraGrouwstra Jan 22, 2017
80f4ca2
don't proceed to git diff on script error
KiaraGrouwstra Jan 22, 2017
724b67b
export interfaces
KiaraGrouwstra Jan 22, 2017
4708550
add a verbose mode showing code
KiaraGrouwstra Jan 22, 2017
e0ba655
space after colon
KiaraGrouwstra Jan 22, 2017
22e340e
mention options in readme
KiaraGrouwstra Jan 22, 2017
a8bedf2
incorporate feedback
KiaraGrouwstra Jan 23, 2017
0f1eaa1
only make host once, separate src/dist
KiaraGrouwstra Jan 23, 2017
6121f0a
update entry path (dist)
KiaraGrouwstra Jan 23, 2017
effc3d8
use local tsc
KiaraGrouwstra Jan 23, 2017
bd225a8
test wrong file path
KiaraGrouwstra Jan 23, 2017
c83ea29
remove cli options (keep lines, keep snippets)
KiaraGrouwstra Jan 23, 2017
9069d3e
update js
KiaraGrouwstra Jan 23, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ An `$ExpectType` directive tests the type of the expression on the next line. Th
npm install -g typings-checker
typings-checker your-test.ts

## options

* `--noLines` (`-l`): hide line numbers in output -- useful if you don't want these clogging diff logs when you add or remove a line.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the use case for this a little more? I'd expect most users of typings-checker would just care about the exit code. As an analogy, the tsc command doesn't have an option to suppress line numbers. Ideally we'd just have a single output format that's good for everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So line numbers are good info -- either way you'll wanna know what code just broke.
The thing here was, like TS and like you, I'm storing the output in git so as to be able to see the diffs.
The issue I wanted to solve here was, if that output log in git contains the line numbers, once you add/remove a line near the start (or whatever), all the line numbers would change, and your diff log would be clogged with a lot of noise (one line per type check), which could crowd out the useful info.
The alternative, showing code snippets, could also change of course, though at least not as easily in all places at once. That's why I added those two options.
Perhaps alternatively it might work to separate things such that info printed to the user and info printed to the diff log would differ here, maybe using stdout vs. stderr.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My inclination would be to just leave both of these options out. If you want to filter out line numbers, you can do it with a bash one-liner:

typings-checker test.ts | perl -pe 's/\.ts:(\d+)/.ts/'

But I still don't totally understand the use case. Wouldn't you always want zero assertion failures, just like you wouldn't want to commit code that doesn't compile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering out the line numbers that way seems like an interesting alternative to me. It hadn't occurred to me, though I suppose I'd felt the option might help people in a similar situation -- which I hope wouldn't be many. Including the code fragments was just a way to make it useful even without line numbers.

Zero assertion failures sounds wonderful to me. I do want that, but that's not where we are for ramda right now; there are quite some blocking issues preventing me from reaching that point even if I wanted to: erased generics, lack of support for heterogeneous map(), lack of type-level support for reduce(), quite possibly support for higher-kinded types, not to mention whatever other features this list might depend on, aside from just finding the time to figure out every single step between here and proper type inference.

I mean, TypeScript has 1,800+ outstanding issues at the moment, so in that sense, I fear it's no surprise to find that not everything is quite perfect yet. FP libraries are probably among the harder ones to type, but I'm probably also being more ambitious than e.g. the lodash typings -- if I map over an object, I'd like for my IDE to know both the (distinct) types and keys of each value. Settling for vague types is an easy way to pass all tests (as you noted), but in a sense feels to me like it beats the point of doing a typings library.

To me this was also the reason to commit and diff the output, like you were. Up to now I had no idea if an edit would break inference and turned everything into any. With your library, now I do. But I need to be able to answer bigger questions than that; notably: given my current assertion failures (whether any or errors), would switching to an experimental branch make that overall situation better or worse?

I also hope I can get to the point of having the luxury to get things fixed. And I'm about to find out more about that -- knowing what's good or bad now, I can finally start seeing more about that.

* `--verbose` (`-v`): shows code samples around your errors. could be used as an alternative to the above.

## Development

```
Expand Down
10 changes: 8 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"description": "Positive and negative assertions about TypeScript types and errors",
"main": "src/index.js",
"scripts": {
"prepublish": "tsc ./src/index.ts",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsc alone should be sufficient here -- the tsconfig.json will tell it what to compile.

"test": "./update-tests.sh"
},
"bin": {
Expand All @@ -21,12 +22,17 @@
"@types/lodash": "^4.14.47",
"@types/mocha": "^2.2.36",
"@types/node": "^7.0.0",
"@types/yargs": "^6.5.0",
"chai": "^3.5.0",
"mocha": "^3.2.0",
"ts-node": "^2.0.0"
"ts-node": "^2.0.0",
"typescript": "^2.1.4"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We import symbols from typescript, so it doesn't make sense for it to be a devDependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intention: ensuring users would not be locked in to a specific TS version (peer dependency). Evidently it would still be needed at run-time though. This was my idea of addressing that.
I'm open to ideas, just hope we wouldn't need to force every single DefinitelyTyped repo to force adhering one and the same TypeScript version.
For Ramda, for one, I'm highly dependent on fixes to the outstanding problems in TypeScript. I'd actually consider trying out different branches / forks for that.

},
"dependencies": {
"lodash": "^4.17.4",
"typescript": "^2.1.4"
"yargs": "^6.6.0"
},
"peerDependencies": {
"typescript": "2.x"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the effect of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peer dependency: see above.
Yargs: I used this to parse command-line options (not order-dependent like the earlier process.argv method).

}
}
80 changes: 22 additions & 58 deletions src/checker.ts
Original file line number Diff line number Diff line change
@@ -1,59 +1,6 @@
import * as ts from 'typescript';
import * as _ from 'lodash';

interface TypeAssertion {
kind: 'type';
type: string;
line: number; // line that the assertion applies to; 0-based.
}

interface ErrorAssertion {
kind: 'error';
pattern: string;
line: number; // line that the assertion applies to; 0-based.
}

type Assertion = TypeAssertion | ErrorAssertion;

export interface NodedAssertion {
assertion: Assertion;
node: ts.Node;
type: ts.Type;
error?: ts.Diagnostic;
}

interface LineNumber {
line: number; // 0-based
}

export interface WrongTypeFailure extends LineNumber {
type: 'WRONG_TYPE';
expectedType: string;
actualType: string;
}

export interface UnexpectedErrorFailure extends LineNumber {
type: 'UNEXPECTED_ERROR';
message: string;
}

export interface WrongErrorFailure extends LineNumber {
type: 'WRONG_ERROR';
expectedMessage: string;
actualMessage: string;
}

export interface MissingErrorFailure extends LineNumber {
type: 'MISSING_ERROR';
message: string;
}

type Failure = WrongTypeFailure | UnexpectedErrorFailure | WrongErrorFailure | MissingErrorFailure;

export interface Report {
numSuccesses: number;
failures: Failure[];
}
import { Assertion, TypeAssertion, ErrorAssertion, Failure, WrongTypeFailure, UnexpectedErrorFailure, WrongErrorFailure, MissingErrorFailure, NodedAssertion, Report } from './types';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this to one symbol per line or use import * as types from './types';


// Extract information about the type/error assertions in a source file.
// The scanner should be positioned at the start of the file.
Expand Down Expand Up @@ -102,6 +49,7 @@ export function attachNodesToAssertions(
const pos = node.getStart();
const { line } = source.getLineAndCharacterOfPosition(pos);
const assertionIndex = _.findIndex(assertions, {line});
// console.warn(`checked for assertions at line ${line}: ${assertionIndex}`);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debug code

if (assertionIndex >= 0) {
const assertion = assertions[assertionIndex];
const type = checker.getTypeAtLocation(node.getChildren()[0]);
Expand All @@ -116,7 +64,15 @@ export function attachNodesToAssertions(

collectNodes(source);
if (assertions.length) {
console.error(assertions);
let prettyAssertions = assertions.map(o => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be written more directly and with fewer type casts:

const prettyAssertions = assertions.map(assertion => {
  let msg: string;
  if (assertion.kind === 'type') {
    msg = `$ExpectType ${assertion.type}`;
  } else {
    msg = `$ExpectError ${assertion.pattern}`
  }
  return `{assertion.line + 1}: ${msg}`;
});

const errorMap: { [k: string]: string } = {
type: `$ExpectType ${(<TypeAssertion>o).type}`,
error: `$ExpectError ${(<ErrorAssertion>o).pattern}`,
};
let msg: string = <string> errorMap[o.kind];
return `${o.line+1}: ${msg}`;
});
console.error(JSON.stringify(prettyAssertions, null, '\t'));
throw new Error('Unable to attach nodes to all assertions.');
}

Expand All @@ -127,13 +83,14 @@ export function generateReport(
checker: ts.TypeChecker,
nodedAssertions: NodedAssertion[],
diagnostics: ts.Diagnostic[],
source: ts.SourceFile,
): Report {
const failures = [] as Failure[];
let numSuccesses = 0;

// Attach errors to nodes; if this isn't possible, then the error was unexpected.
for (const diagnostic of diagnostics) {
let { line } = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start);
let line = diagnostic.file && diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start).line;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is there no file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where I got it: errors related to unresolved types.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test that triggers this case?


const nodedAssertion = _.find(nodedAssertions, {assertion: {line}});
if (nodedAssertion) {
Expand All @@ -149,13 +106,18 @@ export function generateReport(
}

// Go through and check all the assertions.
for (const {assertion, type, error} of nodedAssertions) {
for (const noded of nodedAssertions) {
let { assertion, type, error, node } = noded;
let { pos, end } = node;
let code = source.text.substring(pos, end);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same as node.getText()?

If not, I always prefer to use string.slice. See http://www.danvk.org/2014/11/17/javascript-string-splice-substr-substring-which-to-use.html

const line = assertion.line;
// let base = { code, line };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

if (assertion.kind === 'error') {
if (!error) {
failures.push({
type: 'MISSING_ERROR',
line,
code,
message: assertion.pattern
});
} else {
Expand All @@ -164,6 +126,7 @@ export function generateReport(
failures.push({
type: 'WRONG_ERROR',
line,
code,
expectedMessage: assertion.pattern,
actualMessage: message,
});
Expand All @@ -177,6 +140,7 @@ export function generateReport(
failures.push({
type: 'WRONG_TYPE',
line,
code,
expectedType: assertion.type,
actualType,
})
Expand Down Expand Up @@ -204,5 +168,5 @@ export default function checkFile(
): Report {
const assertions = extractAssertions(scanner, source);
const nodedAssertions = attachNodesToAssertions(source, checker, assertions);
return generateReport(checker, nodedAssertions, diagnostics);
return generateReport(checker, nodedAssertions, diagnostics, source);
}
31 changes: 25 additions & 6 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,37 @@
*/

import * as ts from 'typescript';
let argv = require('yargs')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use import instead of require

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started with import, but it seems they use this syntax for getting in those extra configuration methods before the actual method extraction. At that point I chose to just follow their way, but open to suggestions.

.usage('Usage: <file> [options]')

.alias('l', 'noLines')
.describe('l', 'skip line numbers in output to keep it more diff-friendly')

.alias('v', 'verbose')
.describe('v', 'also print relevant code')

.argv;

import checkFile from './checker';

const [,, tsFile] = process.argv;
const [tsFile] = argv._;
const { noLines, verbose } = argv;

// TODO: read options from a tsconfig.json file.
const options: ts.CompilerOptions = {};
const host = ts.createCompilerHost(options, true);
// read options from a tsconfig.json file.
let host = ts.createCompilerHost({}, true);
const options: ts.CompilerOptions = ts.readConfigFile('tsconfig.json', (path: string) => host.readFile(path)).config['compilerOptions'];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the tests now run with typings-checker's own tsconfig.json? That's probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the consuming project's tsconfig. To me that was the point here, making it configurable to match how a project would want to get compiled.

// console.log(options);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

if (options) {
host = ts.createCompilerHost(options, true);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now, but eventually we should do something more like tslint.

}

const program = ts.createProgram([tsFile], options, host);

const source = program.getSourceFile(tsFile);
if (!source) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this possible? (i.e. can this fail but ts.createProgram succeeds?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if the file path is wrong.

console.error(`could not load content of ${tsFile}`);
process.exit(1);
}
const scanner = ts.createScanner(
ts.ScriptTarget.ES5, false, ts.LanguageVariant.Standard, source.getFullText());

Expand All @@ -31,7 +50,7 @@ const diagnostics = ts.getPreEmitDiagnostics(program);
const report = checkFile(source, scanner, checker, diagnostics);

for (const failure of report.failures) {
const { line } = failure;
const { line, code } = failure;
let message: string;
switch (failure.type) {
case 'UNEXPECTED_ERROR':
Expand All @@ -47,7 +66,7 @@ for (const failure of report.failures) {
message = `Expected type\n ${failure.expectedType}\nbut got:\n ${failure.actualType}`;
break;
}
console.error(`${tsFile}:${line + 1}: ${message}\n`);
console.error(`${tsFile}:${noLines ? '' : ((line + 1) + ':')}${verbose && code ? code + '\n\n' : ' '}${message}\n`);
}

const numFailures = report.failures.length;
Expand Down
62 changes: 62 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import * as ts from 'typescript';

export interface LineNumber {
line: number; // 0-based
}

export interface IAssertion extends LineNumber {
kind: string;
}

export interface TypeAssertion extends IAssertion {
kind: 'type';
type: string;
}

export interface ErrorAssertion extends IAssertion {
kind: 'error';
pattern: string;
}

export type Assertion = TypeAssertion | ErrorAssertion;

export interface NodedAssertion {
assertion: Assertion;
node: ts.Node;
type: ts.Type;
error?: ts.Diagnostic;
}

export interface IFailure extends LineNumber {
code?: string;
type: string;
}

export interface WrongTypeFailure extends IFailure {
type: 'WRONG_TYPE';
expectedType: string;
actualType: string;
}

export interface UnexpectedErrorFailure extends IFailure {
type: 'UNEXPECTED_ERROR';
message: string;
}

export interface WrongErrorFailure extends IFailure {
type: 'WRONG_ERROR';
expectedMessage: string;
actualMessage: string;
}

export interface MissingErrorFailure extends IFailure {
type: 'MISSING_ERROR';
message: string;
}

export type Failure = WrongTypeFailure | UnexpectedErrorFailure | WrongErrorFailure | MissingErrorFailure;

export interface Report {
numSuccesses: number;
failures: Failure[];
}
2 changes: 1 addition & 1 deletion tests/basics/type-assertion-failure.ts.out
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
tests/basics/type-assertion-failure.ts:6: Expected type
tests/basics/type-assertion-failure.ts:6:: Expected type
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The :: double colon doesn't look right (here and elsewhere)

string
but got:
number
Expand Down
2 changes: 1 addition & 1 deletion tests/basics/unexpected-error.ts.out
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
tests/basics/unexpected-error.ts:5: Unexpected error
tests/basics/unexpected-error.ts:5:: Unexpected error
Property 'c' does not exist on type '{ a: string; b: number; }'.

tests/basics/unexpected-error.ts: 0 / 1 checks passed.
16 changes: 11 additions & 5 deletions tests/underscore-any.ts.out
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
tests/underscore-any.ts:7: Expected type
tests/underscore-any.ts:7:: Expected type
number
but got:
any

tests/underscore-any.ts:9: Expected error Operator '==' cannot be applied to types 'number' and 'string'.
tests/underscore-any.ts:9:: Expected error
Operator '==' cannot be applied to types 'number' and 'string'.
but got:
Parameter 'x' implicitly has an 'any' type.

tests/underscore-any.ts:11: Expected type
tests/underscore-any.ts:11:: Expected type
number
but got:
any

tests/underscore-any.ts:13: Expected error Property 'y' does not exist on type '{ x: number; }'.
tests/underscore-any.ts:13:: Expected error
Property 'y' does not exist on type '{ x: number; }'.
but got:
Parameter 'v' implicitly has an 'any' type.

tests/underscore-any.ts:15: Expected type
tests/underscore-any.ts:15:: Expected type
{ x: number; }
but got:
any
Expand Down
1 change: 1 addition & 0 deletions update-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ set +o errexit

for test in $(find tests -name '*.ts'); do
node src/index.js $test > $test.out 2>&1
rc=${PIPESTATUS[0]}; if [[ $rc != 0 ]]; then exit $rc; fi
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here? If there are errors, the checker should fail. But sometimes that's expected. So it shouldn't be a test failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presently it'd continue to run the git commands even if the test could not be completed. This addresses that, just existing with error code if it failed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for some of the tests (e.g. tests/underscore-any.ts), failure is expected. Does this script error out when you run it locally?

done

# This shows changes and sets the exit code.
Expand Down