Skip to content

Add source tests #7

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

Merged
merged 5 commits into from
Mar 27, 2016
Merged

Add source tests #7

merged 5 commits into from
Mar 27, 2016

Conversation

unional
Copy link
Collaborator

@unional unional commented Mar 27, 2016

Overall experience is very good.
Easy to do, catch bunch of errors.

@blakeembrey there is an issue I don't know how to fix. Currently it fails the tests:

utilities.ts (46,23): Only a void function can be called with the 'new' keyword. (2350)
utilities.ts (309,14): Only a void function can be called with the 'new' keyword. (2350)
utilities.ts (449,11): Only a void function can be called with the 'new' keyword. (2350)
utilities.ts (484,15): Only a void function can be called with the 'new' keyword. (2350)

It's the chai.Assertion. It probably should be defined as a class, but I don't know how to convert it.

Can you help me on that?

@unional unional merged commit 7df548e into master Mar 27, 2016
@unional unional deleted the add-source-tests branch March 27, 2016 06:46
@blakeembrey
Copy link
Member

Make sure you gitignore source-test and anything else generated.

@blakeembrey
Copy link
Member

We're talking about https://github.com/typed-typings/npm-chai/blob/master/lib/Assertion.d.ts#L5 right? You can make it an interface with both new and not new, should work:

export interface AssertionStatic {
  (target?: any, message?: string, stack?: Function): Assertion;
  new (target?: any, message?: string, stack?: Function): Assertion;
}

Something else you can do that might be worth doing, is making these types into value representations too. But it's a big refactor to do globally and more an FWIW right now.

export var Assertion: {
  (target?: any, message?: string, stack?: Function): Assertion;
  new (target?: any, message?: string, stack?: Function): Assertion;
}

However, then you need to use typeof to get the value when aliasing it later.

@unional
Copy link
Collaborator Author

unional commented Mar 27, 2016

Make sure you gitignore source-test and anything else generated.

source-test currently is not auto-generated. Typings author will need to:

  • manually copy them from source (that's why having source in submodule make it easy for them)
  • make some changes to make it work:
    • Adding import statements if needed
    • Put in (<obj> as any).dynamicField as needed
    • Comment out or remove negative tests (where the test guard against wrong calling params)

Something else you can do that might be worth doing, is making these types into value representations too.

I was considering to make it into export class Assertion. Would that be a possibility? Since chai didn't use ES6 class but regular JS prototypal inheritance, should I do that?

As for value representation, what is the pros for that? Is it because it shouldn't be a class and var is the next best option?

@blakeembrey
Copy link
Member

ES6 classes are prototypal inheritance, it's just sugar and you can probably read about online somewhere. It can definitely be a class, no advantage of the var over class. You just can't do multiple inheritance with classes which I noticed is currently used with the interfaces - you'll need to refactor that.

@blakeembrey
Copy link
Member

On the source-test, we should try to figure this out - copying and pasting will mean we're outdated often. That may or may not be an issue. Can we apply basic replacement for the import and use --allowJs for the rest (assuming we fix any issues there)?

@unional
Copy link
Collaborator Author

unional commented Mar 27, 2016

ES6 classes are prototypal inheritance,

Yes, I know. What I am considering is whether to write the typing more accurately (staying close to the source, not using class as it is a ES6 construct) or stay true to the shape/intent (using class as that's what it tries to be).

You just can't do multiple inheritance with classes

Yeah, that's the pain point. And it make even less sense in typings. a mixin construct would be the best.

Can we apply basic replacement for the import and use --allowJs for the rest (assuming we fix any issues there)?

Currently I don't see a viable option for this yet. e.g. currently I'm manually adding import chai = require('chai'); to the files because their test harness have manual steps that put chai as global.
Also, there are other cases we need to handle:
Adding any:
https://github.com/typed-typings/npm-chai/blob/master/source-test/should.ts#L842-L844
Commenting out or removing invalid call (these can't be done automatically):
https://github.com/typed-typings/npm-chai/blob/master/source-test/should.ts#L906-L912

Other packages would have their own test harness implementation.

...assuming we fix any issues there

Do you mean making changes directly under source/test/abc.js and local branching the submodule?
I think this could confuse a lot of typings authors.

If we can tap into salsa and configure it to ignore certain rules (e.g. noImplicitAny: true), then we don't need the source-test to begin with. 😄

@blakeembrey
Copy link
Member

You could disable noImplicitAny for the typings projects anyway, I don't think it gets validated either way at a definition level. Only in tests.

Do you mean making changes directly under source/test/abc.js and local branching the submodule?

No, just making it a copy/paste reproducible script. Something that can pull from the source and make changes in an automated fashion (like applying a patch on top).

@blakeembrey
Copy link
Member

Anyway, it's a really cool experiment 👍 Nice to see where this can go!

@unional
Copy link
Collaborator Author

unional commented Mar 28, 2016

Just discover this:
microsoft/TypeScript#3755

interface Model {
  name: string;
  [others: string]: any;
}
function createModel(x: Model) { ... }

// OK
createModel({name: 'hello', length: 100});

Really need to put this in some docs!

@unional
Copy link
Collaborator Author

unional commented Mar 28, 2016

interface DataModelOptions {
  name?: string;
  id?: number;
}
interface UserProperties {
  [key: string]: any;
}
function createDataModel(model: DataModelOptions & UserProperties) {
 /* ... */
}
// findDataModel can only look up by name or id
function findDataModel(model: DataModelOptions) {
 /* ... */
}
// OK
createDataModel({name: 'my model', favoriteAnimal: 'cat' });
// Error, 'ID' is not correct (should be 'id')
findDataModel({ ID: 32 });

Also didn't know & operator on types is possible.

@blakeembrey
Copy link
Member

Yes, & is an intersection of two types and | is the union 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants