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.

Sign up for GitHub

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

Jump to bottom

misc edits #13

Closed
wants to merge 28 commits into from
Closed

misc edits #13

Show file tree
Hide file tree
Changes from all 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
View file
Open in desktop
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
155 changes: 155 additions & 0 deletions dist/checker.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions dist/checker.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 46 additions & 0 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions dist/index.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions dist/types.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions dist/types.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 10 additions & 5 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@
"name": "typings-checker",
"version": "1.1.0",
"description": "Positive and negative assertions about TypeScript types and errors",
"main": "src/index.js",
"main": "dist/index.js",
"scripts": {
"prepublish": "tsc",
"test": "./update-tests.sh"
},
"bin": {
"typings-checker": "src/index.js"
"typings-checker": "dist/index.js"
},
"author": "Dan Vanderkam (danvdk@gmail.com)",
"license": "ISC",
Expand All @@ -21,12 +22,16 @@
"@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"
"lodash": "^4.17.4"
},
"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).

}
}
Loading