add typedefs for src/utils/is.js #2432

ChristopherChudzicki · 2022-02-19T14:01:05Z

Add simple typedefs for most of the functions in src/utils/is.js. My immediate motivation was a desire for isMatrix, which was missing a type definition. But I just added the typedefs for all the isWhatever functions.

Note: The existing isThing typedefs seem to all be for the functions in src/function/utils, e.g., isNumeric.

PS: As always, thanks for mathjs. I continue to be amazed at how you maintain this project in such a positive way.

gwhitney · 2022-02-21T22:17:58Z

As mentioned in #2431, I think it's putting the cart before the horse a bit to have TypeScript definitions for these functions when they are not even officially published as part of the mathjs API. When we get a response on whether that's a direction mathjs will go, hopefully you can expand this PR to one that makes them full-fledged functions.

That will presumably also involve moving unit tests for these into a location corresponding to their current location in the source (see the TODO at the top of test/unit-tests/core/typed.test.js), and if any are missing unit tests (not sure if any are), adding unit tests for those in the correct place.

Is there any reasonable way we can unit test the TypeScript type declarations for mathjs themselves? If so, it would probably be wise to add such tests, as TypeScript issues pop up with fair regularity.

Previously, the internal mathjs configuration was not picking up the typedefs for `decimal.js`. Consequently, Decimal was being typed as "any", allowing erroneous tests to pass in `index.ts`.

ChristopherChudzicki · 2022-03-10T13:17:12Z

@gwhitney Came back to this last night. As discussed in #2444 , the goal of this current PR is to make the type definitions for mathjs more accurately reflect its current exports. To that end, I've added typedefs for all the src/util/is isX functions and made Matrix useable as a constructor.

Glad you opened #2472 else I would not have realized test:types exists now, so I added some simple tests for these new types, too.

josdejong · 2022-03-11T09:00:13Z

Thanks @ChristopherChudzicki for working this out, I see in the unit tests all the type guards work 😎.

I think this PR is ready to be merged, agree @gwhitney ?

types/index.d.ts

gwhitney · 2022-03-15T20:54:28Z

I think this PR is ready to be merged, agree @gwhitney ?

Ok, so I did a review as I took this to be a request for, just a few comments above. And as far as the big picture goes, let me recap to make sure we are all on the same page:

We have decided to keep all of the currently exported "is" functions exported as bona fide mathjs functions.
Therefore, they need (a) public documentation (not just in-code comments), (b) to be available in the parser, (c) embedded docs, and (d) TypeScript types.
We have also decided that bona fide mathjs functions, available directly and in the parser, can quite reasonably be ordinary JavaScript functions, not necessarily always typed-functions, and in particular these "is" functions will remain ordinary JavaScript functions, quite possibly "forever."
Finally, we have decided to split up the "regularization" of these functions into multiple PRs: this one will serve for (d) above, and one or more others will take care of (a)-(c).

If you agree with all those points (I am OK with them) then I think except for the review questions and the fact that I'd suggest that an issue calling for (a)-(c) for the "is" functions be filed before this is merged, I think this is ready to go. I am happy to file the issue when I hear back on this comment; or in fact I think it would be perfectly sensible to morph #2444 into exactly that issue. And @ChristopherChudzicki any chance you're able to try your hand at a PR for some or all of (a)-(c)? It would really help since as you can see there is a lot on the mathjs plate at the moment.

josdejong · 2022-03-16T08:17:58Z

Thanks for your clear summary @gwhitney 👍 . Yes please open a new issue to address the things that still need to be addressed after this PR is merged.

gwhitney · 2022-03-17T22:33:52Z

On Thu, 2022-03-17 at 15:21 -0700, Chris Chudzicki wrote: @ChristopherChudzicki commented on this pull request. In types/index.d.ts: > + isMatrix(x: unknown): x is Matrix; + + isCollection(x: unknown): x is (Matrix | any[]); + + isDenseMatrix(x: unknown): x is Matrix; + + isSparseMatrix(x: unknown): x is Matrix; + + isRange(x: unknown): boolean; + + isIndex(x: unknown): x is Index; + + isBoolean(x: unknown): x is boolean; + + isResultSet(x: unknown): boolean; + I'd prefer to add that in a separate PR, which I'm happy to do. The main reason I didn't include it was because it didn't a type declared and I wasn't particularly familiar with it. I've made isChain a type predicate since that already has a type defined. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: ***@***.***>

I got this email but don't actually see the comment or the new commit on GitHub, which seems very weird to me. But in any case, this all seems reasonable to me and when the commit does show up (don't forget fixing the whitespace nit as well) I will be happy to merge it and file an issue with all the outstanding tasks (which yes it would be great if you could help pitch in on, Christopher).

gwhitney · 2022-03-17T22:40:04Z

I see it now and have filed issue #2487 so I will go ahead and merge.

ChristopherChudzicki · 2022-03-17T22:53:51Z

@gwhitney Hehe I deleted that comment because I decided to include it as part of an overall comment, so I could collocate the responses about Range and ResultSet. But... I shouldn't have deleted it. I knew that email would be confusing 🤦

@gwhitney Thanks for the review, I've updated the PR to make isChain a type predicate and removed the extra whitespace. I'd prefer to keep larger changes (like defining new Range and ResultSet types) for a subsequent PR. I'm happy to take a stab at that this weekend.

My take-aways from the PR / issue discussions is slightly different. In my opinion:

Types should accurately reflect the current exports of MathJS: Whether something appears in the markdown documentation or not, if it's exported, it's part of the public API and having types available for exported stuff is useful. (It might not be an important part of the public API, in which case removing it in a major release is likely not very disruptive.)

As an example of this, the src/util/is functions are exported, so their types should be accurate.

Take this with a grain of salt. I believe this 99%. But if something really silly was exported by accident, fix it in a patch
Runtime type checking for these functions is not worth it: Since they should never throw errors.
Most exported functions should be accessible in the expression parser: These functions are currently not. But note that some other utility-esque functions are also not available: create, parse, evaluate are three examples.

Definitely the isX functions have a slightly different flavor than create and parse... isX could reasonably be included, whereas I don't see how those other ones could, so I think including them is very reasonable.

In regards to documentation for these isX functions... I actually do not think it's worth it, especially if MathJS documentation is handwritten Markdown files (which it appears to be). The isX functions are the kind of thing I would include in an auto-generated API reference. Anyone consuming the isMatrix function probably understands what it does.

Although... I guess including in the docs helps discoverability. It would be really nice if the docs could be auto-generated, though...

IMO, next steps should be:

Next steps:

Either:
- Make isX etc available in expression parser! Probably not that useful, but
- OR: remove them (in a major release) from top-level exports, and make them available only via
```
import { isMatrix } from mathjs/util
```
  or something like that.
Add types for ResultSet and Range
Keep improving the types!
Totally unrelated but has been annoying me: Set up linting for the typescript files! Or maybe I just have it configured wrong locally.

I'm very happy to help with any and all of those (and elsewhere in MathJs).

ChristopherChudzicki · 2022-03-17T23:01:15Z

@gwhitney I should add: Those were my takeaways, but your plan sounds reasonable, too, and I am very happy to contribute to #2487 . The steps in #2487 are very reasonable and useful. Probably better to just do that than dither for a long time, which is my usual mode of operation.

gwhitney · 2022-03-17T23:04:12Z

Since I am about to close this, you should probably port your further commentary to #2487. However, @josdejong has confirmed that the desired invariant is:

mathjs function exports === web-documented functions ==~ functions in parser,

where the last equality is just slightly approximate because of a tiny list of items like "math.create" which don't really make sense in the parser context. But virtually everything else does, including parse and evaluate, which are in fact currently available in the parser.

Also, Jos has officially blessed the "is" functions. Therefore, as recorded in #2487, they do need to be available in the parser, and have JSDoc documentation, which should generate the website documentation, and they should have embedded docs. Help with any/all of these tasks is very welcome! Thanks for this step :)

…ntains only one element (#2422) * Fix #2412: let function diff return an empty matrix when the input has only one element * Undo changes in History in this fixme * Add TypeScript definitions for src/utils/is.js (#2432) This is a first step toward full publication of these functions, that were already being exported by mathjs but had not yet had the associated actions (documentation/available in parser/typed, etc.) Also, makes most of them into TypeScript type guards, and adds Matrix as a constructor type. Resolved #2431. Co-authored-by: Glen Whitney <glen@studioinfinity.org> * test: add two-dimensional test cases for diff of length 1 Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com> Co-authored-by: Glen Whitney <glen@studioinfinity.org>

josdejong · 2022-03-23T11:29:09Z

Published in v10.4.1

…ntains only one element (#2422) * Fix #2412: let function diff return an empty matrix when the input has only one element * Undo changes in History in this fixme * Add TypeScript definitions for src/utils/is.js (#2432) This is a first step toward full publication of these functions, that were already being exported by mathjs but had not yet had the associated actions (documentation/available in parser/typed, etc.) Also, makes most of them into TypeScript type guards, and adds Matrix as a constructor type. Resolved #2431. Co-authored-by: Glen Whitney <glen@studioinfinity.org> * test: add two-dimensional test cases for diff of length 1 Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com> Co-authored-by: Glen Whitney <glen@studioinfinity.org>

* refactor: Remove the automatic conversion from number to string. (#2482) This is a breaking change. However, nothing in the unit tests or examples actually depended on such a conversion, and it's difficult to construct situations in which it's necessary. The best such example is e.g. `count(57)` which formerly gave the number of digits in its numeric argument. Of course, after this commit, that behavior can still be obtained by the just slightly longer expression `count(string(57))` The change is proposed in preparation for an addition of new facilities/ handlers to allow symbolic computation in a couple of different ways (see #2475 and #2470). * feat(simplifyCore): convert equivalent function calls into operators (#2466) * feat(simplifyCore): convert equivalent function calls into operators Resolves #2415. * docs: Every operator has a function form Also documents the new behavior of simplifyCore to convert function calls into any equivalent operator form they may have. Also fixes the syntax errors so that simplifyCore will successfully doctest. * docs: Fix table syntax for operator->function correspondence * fix(parse): Implement amended "Rule 2" As per the discussion in #2370, the amended "Rule 2" is "when having a division followed by an implicit multiplication, the division gets higher precedence over the implicit multiplication when (a) the numerator is a constant with optionally a prefix operator (-, +, ~), and (b) the denominator is a constant." This commit implements that behavior and adds tests for it. Resolves #2370. * fix: OperatorNode.toString() outputs match implicit multiplication parsing Also greatly extends the tests on OperatorNode.toString() and .toTex(), and ensures that all tests are performed on both. (toHTML() is still a testing stepchild.) Also fixes other small bugs in .toString() and .toTex() revealed by the new tests. Resolves #1431. * test(parse): More cases of implicit multiplication * refactor: Alter the precedence of implicit multiplication This greatly simplifies OperatorNode:calculateNecessaryParentheses, as opposed to trying to correct for the change in precedence after the fact. * Fix broken unit test * Replace `options && options.implicit` with `options?.implicit` * Replace `options?.implicit` with `options && options.implicit` again, it breaks the Node 12 tests * chore: Prevent confusion with standard matrix functions. (#2465) * chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement. * Fix #2412: let function diff return an empty matrix when the input contains only one element (#2422) * Fix #2412: let function diff return an empty matrix when the input has only one element * Undo changes in History in this fixme * Add TypeScript definitions for src/utils/is.js (#2432) This is a first step toward full publication of these functions, that were already being exported by mathjs but had not yet had the associated actions (documentation/available in parser/typed, etc.) Also, makes most of them into TypeScript type guards, and adds Matrix as a constructor type. Resolved #2431. Co-authored-by: Glen Whitney <glen@studioinfinity.org> * test: add two-dimensional test cases for diff of length 1 Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com> Co-authored-by: Glen Whitney <glen@studioinfinity.org> * Refactor/simplify core cleanup (#2490) * refactor: don't simplify constants in simplifyCore Keeps the operation of simplifyCore cleanly separate from simplifyConstant. * fix; handle multiple consecutive operations in simplifyCore() Also adds support for logical operators. Resolves #2484. * feat: export simplifyConstant Now that simplifyCore does not do any constant folding, clients may wish to access that behavior via simplifyConstant. Moreover, exporting it makes it easier to use in custom rule lists for simplify(). Also adds docs, embedded docs, and tests for simplifyConstant(). Also fixes simplifyCore() on logical functions (they always return boolean, rather than "short-circuiting"). Resolves #2459. * refactor: Rename matrix algorithms to stay sane in next refactor * refactor: Create a generator for boilerplate matrix versions of operations This reduces code length and duplication, and significantly reduces the number of instances of 'this' that will require replacement when moving on top of typed-function v3. * refactor: add automatic conversion from string to Node Eliminates many `this` calls in src/function/algebra, which will help conversion to typed-function v3a. Also make `resolve` into a typed function so that it will now work on strings as well, and adds a test that it does. * refactor: Use temporary conversions to simplify typed-function definitions Specifically, temporarily converting Object to Map eases the definition of 'simplify' and a new, generally ignored type 'identifier' (a subtype of 'string') with a temporary conversion to 'SymbolNode' simplifies the definition of 'derivative'. These refactors eliminate multiple instances of this, which will ease conversion to typed-function v3a. * refactor: Speed up utils/is.js typeOf function In preparation for using it as the function selector for the Unit class. Also fixes the inconsistency between the `typed` type hierarchy 'function' and typeOf returning 'Function' in favor of 'function', again to minimize the special cases in typeOf * feat(Unit): Add a method giving the (string name of the) type of the value E.g. `math.unit('5cm').valType()` returns `number`. Also uses this for an internal method that directly gives the number converter for a Unit. Also fixes lint errors from previous commit (not clean, I know, I forgot that build-and-test does not run lint). Adds tests for unit.valType() * refactor: Eliminate hyperbolic functions operating on angles There is no mathematical meaning to a hyperbolic function operating on an angle (the proper units of its argument is actually area), and it eliminates a number of uses of `this`, so remove such arguments. * refactor: Remove miscellaneous unnecessary typed-function this refs * refactor: Adapt to typed-function v3a Mostly this involves replaceing instances of 'this' with used of (preferably) typed.referTo() or typed.referToSelf(). Some repeated batterns of boilerpolate signatures within different divisions of functions (bitwise, relational, trigonometry) were factored out into their own files and reused in several of the individual functions. * tests: Only require that derivative tests mention the proper node type * refactor: remove typed.ignore * chore: Update to typed-function 3.0 Also had to deal with new typing for `resolve()` in that it now accepts strings and Matrices; added tests for the new possibilities for `resolve()`, and eliminated empty comments from the Node representation of parsed strings as they can't really be doing anyone any good and they are a pain for testing. Also updates the TypeScript declarations and tests for `resolve()` * chore: Object.hasOwn not supported in Node 14 Also removes 'resolve' from the known failing doc tests, now that it handles strings. * chore: Drop ES5 / IE 11 support. * fix(types): Remove no-longer-implementd matrix overloads * test(identifier): As requested in review item 2 * refactor(Unit): valType => valueType as per review item 3 * test(hasNumericValue): Test boolean arguments as per review item 4 * refactor(Node): Use class syntax rather than assigning prototypes This change simplifies the typeOf() function, because now all subclasses of Node have the expected constructor name. Also, reformats the documentation of the typeOf() function so that the doc test of that function will serve as an exhaustive test that the bundle returns the proper types. * Prevent chain functions from matching stored value with a rest parameter (#2559) * chore: Prevent confusion with standard matrix functions. (#2465) * chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement. * fix: prevent chain from matching rest parameter with stored value Since the revised code needs the isTypedFunction predicate, switch to using the typed-function implementation for that throughout mathjs, rather than rolling our own here. Also adds a test that chain() no longer allows this kind of usage. Removes the two type declarations in types/index.d.ts that were allowing this sort of "split rest" call and added tests that such calls are forbidden. Adds to the chaining documentation page that such "split" calls are not allowed. * chore: Refresh this PR to reflect underlying changes Also addresses the review request with a detailed comment on the correctness of a new code section. Note that it reverts some changes to the TypeScript signatures of the matrix functions ones() and zeros() -- they do not actually have a typed-function signature of two numbers and an optional format specifically for two dimensions. What they have is a single rest parameter, from which the format is extracted if present. Hence, due to the ban on breaking rest parameters, it is not valid to call math.chain(3).zeros(2) to make a 3-by-2 matrix of zeros, which seems like a perfectly valid ban as the division of the dimensions is very confusing; this should be written as math.chain([3,2]).zeros(). The TypeScript signatures are fixed accordingly, along with the edge case of no arguments to ones() and zeros() at all, which does work to produce the "empty matrix". * Unit test `typeOf` on the minified bundle (currently failing) * Update AUTHORS * Improve testing of typeOf on browser bundle (WIP) * fix #2621: Module "mathjs" has no exported member "count" .ts(2305) (#2622) * fix #2621: Module "mathjs" has no exported member "count" .ts(2305) * feat: Update comments of count * feat: update the signature for count * feat: add usage example for count and sum * chore: Ensure type info remains in bundling Co-authored-by: Glen Whitney <glen@studioinfinity.org> Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com> Co-authored-by: Hansuku <1556207795@qq.com>

ChristopherChudzicki mentioned this pull request Feb 22, 2022

Proposal: Expose isMatrix #2444

Closed

ChristopherChudzicki force-pushed the cc/is-types branch from be1554d to b5fa70f Compare March 10, 2022 04:17

ChristopherChudzicki added 3 commits March 10, 2022 08:13

change module resolution to node

860b06b

Previously, the internal mathjs configuration was not picking up the typedefs for `decimal.js`. Consequently, Decimal was being typed as "any", allowing erroneous tests to pass in `index.ts`.

add typedefs for src/utils/is.js

2861317

add Matrix as a constructor type

664a57c

ChristopherChudzicki force-pushed the cc/is-types branch from 1a7ba33 to 664a57c Compare March 10, 2022 13:13

Merge branch 'develop' into cc/is-types

43da605

gwhitney reviewed Mar 15, 2022

View reviewed changes

types/index.d.ts Show resolved Hide resolved

types/index.d.ts Show resolved Hide resolved

types/index.d.ts Show resolved Hide resolved

types/index.d.ts Show resolved Hide resolved

ChristopherChudzicki added 2 commits March 17, 2022 18:30

Merge remote-tracking branch 'upstream/develop' into cc/is-types

e3cbd71

revert whitespace change; make isChain a predicate

6df8613

gwhitney mentioned this pull request Mar 17, 2022

Extend linting to types/ directory #2488

Closed

chore: Update HISTORY, remove trailing whitespace in types/index.ts

8d6f382

gwhitney merged commit 99839a8 into josdejong:develop Mar 17, 2022

ChristopherChudzicki mentioned this pull request Oct 13, 2022

TS regression: Narrowing of MathNodes by type #2810

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add typedefs for src/utils/is.js #2432

add typedefs for src/utils/is.js #2432

ChristopherChudzicki commented Feb 19, 2022 •

edited

Loading

gwhitney commented Feb 21, 2022

ChristopherChudzicki commented Mar 10, 2022

josdejong commented Mar 11, 2022

gwhitney commented Mar 15, 2022

josdejong commented Mar 16, 2022

gwhitney commented Mar 17, 2022 via email

gwhitney commented Mar 17, 2022

ChristopherChudzicki commented Mar 17, 2022 •

edited

Loading

ChristopherChudzicki commented Mar 17, 2022

gwhitney commented Mar 17, 2022

josdejong commented Mar 23, 2022

add typedefs for src/utils/is.js #2432

add typedefs for src/utils/is.js #2432

Conversation

ChristopherChudzicki commented Feb 19, 2022 • edited Loading

gwhitney commented Feb 21, 2022

ChristopherChudzicki commented Mar 10, 2022

josdejong commented Mar 11, 2022

gwhitney commented Mar 15, 2022

josdejong commented Mar 16, 2022

gwhitney commented Mar 17, 2022 via email

gwhitney commented Mar 17, 2022

ChristopherChudzicki commented Mar 17, 2022 • edited Loading

ChristopherChudzicki commented Mar 17, 2022

gwhitney commented Mar 17, 2022

josdejong commented Mar 23, 2022

ChristopherChudzicki commented Feb 19, 2022 •

edited

Loading

ChristopherChudzicki commented Mar 17, 2022 •

edited

Loading