Skip to content

Types/add generics to remaining node types#2733

Merged
josdejong merged 13 commits into
josdejong:developfrom
goodproblems:types/add-generics-to-remaining-node-types
Oct 20, 2022
Merged

Types/add generics to remaining node types#2733
josdejong merged 13 commits into
josdejong:developfrom
goodproblems:types/add-generics-to-remaining-node-types

Conversation

@mattvague
Copy link
Copy Markdown
Contributor

No description provided.

@josdejong
Copy link
Copy Markdown
Owner

Ahh, I see where you want to go. Makes sense. Thanks for starting this PR 👍

@mattvague
Copy link
Copy Markdown
Contributor Author

Ahh, I see where you want to go. Makes sense. Thanks for starting this PR 👍

Yeah we've found having generics on OperatorNode extremely useful so far and would like to extend that to everything else. I believe @isaacbyr is going to be finishing this one off for us :-)

@mattvague mattvague force-pushed the types/add-generics-to-remaining-node-types branch from c4eeba4 to 42e745e Compare September 20, 2022 18:46
@mattvague mattvague marked this pull request as ready for review September 20, 2022 18:46
@josdejong
Copy link
Copy Markdown
Owner

Thanks Matt, this looks good. Two small feedbacks:

  1. The build-and-test script fails with errors (see https://github.com/josdejong/mathjs/actions/runs/3092630770/jobs/5004119664). Can you look into that?

    Error: index.ts(985,5): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(1804,5): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(1806,50): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(1812,5): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(1818,5): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(1824,5): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(1830,5): error TS2554: Expected 1 arguments, but got 0.
    Error: index.ts(2294,27): error TS2554: Expected 1 arguments, but got 0.
    Error: Process completed with exit code 2.
    
  2. How can we verify that the generics work as intended? Can we add a few usage examples in index.ts? (Or is that already covered by fixing (1))?

@mattvague mattvague force-pushed the types/add-generics-to-remaining-node-types branch from 42e745e to da4d826 Compare October 4, 2022 18:02
@mattvague
Copy link
Copy Markdown
Contributor Author

mattvague commented Oct 4, 2022

How can we verify that the generics work as intended? Can we add a few usage examples in index.ts? (Or is that already covered by fixing (1))?

@josdejong I was thinking that the existing examples would probably cover most types but we could probably beef it up a bit. To be honest I guess I also had a bit of a mental block about doing this given how huge index.ts is becoming and how I'd have to make a lot of decisions like "where in this file would these kinds of tests go". I wonder if we want to take this opportunity to start splitting this into smaller files (e.g. one-to-one with the files in src/expression/node/**)?

@josdejong
Copy link
Copy Markdown
Owner

Yes good idea to start splitting the index.ts file!

If you're in the mood for it you could make a small in this PR, but please keep it small, I would love to merge this PR soon and not have an endlessly growing PR :)

@mattvague
Copy link
Copy Markdown
Contributor Author

Yes good idea to start splitting the index.ts file!

If you're in the mood for it you could make a small in this PR, but please keep it small, I would love to merge this PR soon and not have an endlessly growing PR :)

@josdejong Don't think I have the time right now to properly, fully test this. I also kind of think that our existing tests should be sufficient to at least ensure that nothing breaks. Could we merge this and come up with a more comprehensive test plan later?

@mattvague mattvague force-pushed the types/add-generics-to-remaining-node-types branch from da4d826 to ac91a6d Compare October 19, 2022 19:40
@josdejong
Copy link
Copy Markdown
Owner

Definitely! I'll merge this PR now and will publish it tomorrow.

@josdejong josdejong merged commit f79517f into josdejong:develop Oct 20, 2022
@josdejong
Copy link
Copy Markdown
Owner

Published now in v11.3.2

Comment thread types/index.d.ts
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.

3 participants