misc edits #13

KiaraGrouwstra · 2017-01-22T10:34:37Z

prepublish script: tsc
dry interfaces
use local tsconfig.json
add option to hide line numbers
improve debug message on attach failure

Feel free to use/ignore, these seemed useful to me. On that last point... yeah, since 1.1 it's failing to attach for me (using old separate-line assertions) on every single line.

Used: `typings-checker test.ts`. Error: `Cannot find module './checker'`. Not sure why `index.ts` did have a compiled version!

how come I still had this?

- without JSON.stringify it concats the array (fails on all lines for me since 1.1). - converting objects to strings, prettier but mostly prevents the objects from being printed one property per line.

KiaraGrouwstra · 2017-01-22T10:45:52Z

I managed to do a replace to convert my assertions to inline format.
For multi-line expressions though, I feel that the old option breaking is not fully as elegant as also retaining the possibility of keeping the assertion on a separate line.

Edit: just noticed the existing tests for both cases, and can't reproduce there. Odd.

please don't lock me in to a specific version. :(

I bumped into this when the TS type def library isn't loaded properly.

KiaraGrouwstra · 2017-01-22T16:01:56Z

I tried a version of your bash script for ramda too; using variants of that might be of interest to other users as well.

danvk · 2017-01-22T17:08:49Z

Thanks for the changes! I'll review them shortly.

re: inline assertions, they were intended to be in addition to full-line assertions, not a replacement for them. If you have a test file that 1.1 broke, please add it to this repo and let's fix the bug!

KiaraGrouwstra · 2017-01-22T17:13:35Z

Yeah, I noticed the tests afterwards.
For me it seemed like all of them had broken, so I'm not quite sure how that might've happened while yours were all good. I didn't have many multi-line expressions though, so for the time being I'd just converted all of them to in-line checks.

Take your time on the review, and feel free to ask.

danvk

Thanks for the changes -- I left a bunch of comments / questions for you.

danvk · 2017-01-22T17:10:57Z

README.md

@@ -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.


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.

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.

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?

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.

danvk · 2017-01-22T17:12:15Z

package.json

@@ -4,6 +4,7 @@
  "description": "Positive and negative assertions about TypeScript types and errors",
  "main": "src/index.js",
  "scripts": {
+    "prepublish": "tsc ./src/index.ts",


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

danvk · 2017-01-22T17:12:33Z

package.json

+    "yargs": "^6.6.0"
+  },
+  "peerDependencies": {
+    "typescript": "2.x"


What's the effect of this?

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

danvk · 2017-01-22T17:13:21Z

package.json

    "chai": "^3.5.0",
    "mocha": "^3.2.0",
-    "ts-node": "^2.0.0"
+    "ts-node": "^2.0.0",
+    "typescript": "^2.1.4"


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

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.

danvk · 2017-01-22T17:14:37Z

src/checker.ts

-  numSuccesses: number;
-  failures: Failure[];
-}
+import { Assertion, TypeAssertion, ErrorAssertion, Failure, WrongTypeFailure, UnexpectedErrorFailure, WrongErrorFailure, MissingErrorFailure, NodedAssertion, Report } from './types';


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

danvk · 2017-01-22T17:44:33Z

tests/basics/type-assertion-failure.ts.out

@@ -1,4 +1,4 @@
-tests/basics/type-assertion-failure.ts:6: Expected type
+tests/basics/type-assertion-failure.ts:6:: Expected type


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

danvk · 2017-01-22T17:46:58Z

update-tests.sh

@@ -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


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.

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.

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?

danvk · 2017-01-22T17:49:30Z

src/index.ts

@@ -10,18 +10,37 @@
 */

 import * as ts from 'typescript';
+let argv = require('yargs')


danvk · 2017-01-22T17:49:55Z

src/index.ts

@@ -10,18 +10,37 @@
 */

 import * as ts from 'typescript';
+let argv = require('yargs')


use import instead of require

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.

danvk · 2017-01-22T17:50:29Z

src/index.ts

-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'];


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

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.

KiaraGrouwstra · 2017-01-23T13:37:18Z

I've incorporated your feedback for the points where it was clear how.
Note that I'm personally still experiencing a few issues I hope you could take a look at as well:

use of the global tsc in the bash script triggers errors about unrecognized compiler options for me, specifically for noUnusedLocals with global TS version 2.2.0-dev.20161127. I hope you'd be cool with using a local reference there.
for whatever reason my bash script appears to be trying to run the ts files as js even after I've separated them. kinda weird.

danvk · 2017-01-23T16:21:27Z

Let's get rid of the command-line flags, so that the main thrust of this PR is factoring out the types and loading the tsconfig file.

Changing the bash script to reference node_modules/.bin/tsc is fine by me. Alternatively you could invoke it via yarn test and it should run with an updated $PATH.

And a test for the no-file error would be great. I'd like to add tslint to this project but I'll hold off until this is in, since there will presumably be many merge conflicts.

KiaraGrouwstra · 2017-01-23T17:13:24Z

To clarify, would you prefer the code snippets out as well?
i.e., not optional, you do mean keep both behaviors as they were without the flags, correct? The reason I ask is, in the use-case you mentioned (no diffing / intentional errors), I imagine the extra info could potentially still be considered of use to immediately see the relevant bad code from the output.

KiaraGrouwstra · 2017-01-23T17:22:18Z

did those now (currently keeping code); I'll go sleep now, so feel free to edit yourself.

danvk · 2017-01-24T05:23:08Z

Closing in favor of #19.

I dropped the inclusion of code snippets in error messages for now. I also added a --project flag to point at the right tsconfig.json (this was needed to get the tests passing).

KiaraGrouwstra added 9 commits January 20, 2017 14:35

declare process, or I can't compile this

a4234e0

merge

1680235

add prepublish script, fixes missing checker.js

efd20ae

Used: `typings-checker test.ts`. Error: `Cannot find module './checker'`. Not sure why `index.ts` did have a compiled version!

ditch original file.

9bc9613

how come I still had this?

dry interfaces

1ae10d0

use local tsconfig.json

77411da

add option to hide line numbers for diff logs

ed96cc0

in the error message show the 'real' line numbers

be8019a

improve attach failure log

8439093

- without JSON.stringify it concats the array (fails on all lines for me since 1.1). - converting objects to strings, prettier but mostly prevents the objects from being printed one property per line.

KiaraGrouwstra added 8 commits January 22, 2017 19:03

fix superfluous colon

3810735

convert TS to lenient peer dependency.

8894b3c

please don't lock me in to a specific version. :(

re-add TS in dev dependencies as well

c64000c

error out on bad file paths

2680172

handle edge case of diagnostics with undefined file.

94c0cf2

I bumped into this when the TS type def library isn't loaded properly.

ditch superfluous logging statement

8f20488

externalize interfaces

7016983

don't proceed to git diff on script error

80f4ca2

KiaraGrouwstra added 4 commits January 23, 2017 00:04

export interfaces

724b67b

add a verbose mode showing code

4708550

space after colon

e0ba655

mention options in readme

22e340e

danvk requested changes Jan 22, 2017

View reviewed changes

KiaraGrouwstra added 2 commits January 23, 2017 21:12

incorporate feedback

a8bedf2

only make host once, separate src/dist

0f1eaa1

update entry path (dist)

6121f0a

KiaraGrouwstra added 4 commits January 24, 2017 01:17

use local tsc

effc3d8

test wrong file path

bd225a8

remove cli options (keep lines, keep snippets)

c83ea29

update js

9069d3e

danvk mentioned this pull request Jan 24, 2017

Use tsconfig.json, clearer errors for unattached nodes #19

Merged

danvk closed this Jan 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc edits #13

misc edits #13

KiaraGrouwstra commented Jan 22, 2017

KiaraGrouwstra commented Jan 22, 2017 •

edited

Loading

KiaraGrouwstra commented Jan 22, 2017

danvk commented Jan 22, 2017

KiaraGrouwstra commented Jan 22, 2017

danvk left a comment

danvk Jan 22, 2017

KiaraGrouwstra Jan 23, 2017

danvk Jan 23, 2017

KiaraGrouwstra Jan 23, 2017

danvk Jan 22, 2017

danvk Jan 22, 2017

KiaraGrouwstra Jan 23, 2017

danvk Jan 22, 2017

KiaraGrouwstra Jan 23, 2017

danvk Jan 22, 2017

danvk Jan 22, 2017

danvk Jan 22, 2017

KiaraGrouwstra Jan 23, 2017

danvk Jan 23, 2017

danvk Jan 22, 2017

danvk Jan 22, 2017

KiaraGrouwstra Jan 23, 2017

danvk Jan 22, 2017

KiaraGrouwstra Jan 23, 2017

KiaraGrouwstra commented Jan 23, 2017

danvk commented Jan 23, 2017

KiaraGrouwstra commented Jan 23, 2017 •

edited

Loading

KiaraGrouwstra commented Jan 23, 2017

danvk commented Jan 24, 2017

		@@ -1,4 +1,4 @@
		tests/basics/type-assertion-failure.ts:6: Expected type
		tests/basics/type-assertion-failure.ts:6:: Expected type

misc edits #13

misc edits #13

Conversation

KiaraGrouwstra commented Jan 22, 2017

KiaraGrouwstra commented Jan 22, 2017 • edited Loading

KiaraGrouwstra commented Jan 22, 2017

danvk commented Jan 22, 2017

KiaraGrouwstra commented Jan 22, 2017

danvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiaraGrouwstra commented Jan 23, 2017

danvk commented Jan 23, 2017

KiaraGrouwstra commented Jan 23, 2017 • edited Loading

KiaraGrouwstra commented Jan 23, 2017

danvk commented Jan 24, 2017

KiaraGrouwstra commented Jan 22, 2017 •

edited

Loading

KiaraGrouwstra commented Jan 23, 2017 •

edited

Loading