Tests for examples in embedded docs

[dvd101x]

Hi, this includes tests for the examples of the embedded docs according to #3391

[gwhitney]

OK, I checked out the branch and put an error in an example, and indeed the test failed. The code looks fine. So my only question before merging this is that it only runs the check for identifiers in embeddedDocs -- that's OK, we could put the test elsewhere that there is embeded documentation for every identifier callable from the expression evaluator -- but then it gives an identifier foo a pass if help("foo") throws an error. All of those help calls should succeed, and in fact all the tests pass if the try {...} catch { return } block around the evaluation of help("${name}") is removed. So I would strongly recommend removing that try - catch from this PR.

[dvd101x]

Nice catch!

Previously I was checking on math.expression.mathWithTranform and that try-catch was needed but not when checking directly on embeddedDocs.

Fixed it.

[dvd101x]

Also if we check for math.expression.mathWithTransform these are the ones that throws an error when calling math.evaluate("help(name)"). I don't know if they should be documented and maybe should be included in this PR.

• addScalar
• compile
• divideScalar
• equalScalar
• apply
• multiplyScalar
• not
• parse
• parser
• replacer
• reviver
• subtractScalar
• version

[gwhitney]

Thanks for the quick update.

Also if we check for math.expression.mathWithTransform these are the ones that throws an error when calling math.evaluate("help(name)"). I don't know if they should be documented and maybe should be included in this PR.

* `addScalar`, `divideScalar`, `equalScalar`, `multiplyScalar`, `subtractScalar`, `apply`

These are not really intended to be called by clients of the package, and so it's OK for them to be undocumented, and they can be recorded as such.

* `compile`, `not`, `parse`, `parser`, `version`

These should definitely be documented; I will leave it up to @josdejong whether he wants the addition of documentation to be part of this PR, and whether he wants to add a check that everything is documented besides exceptions like the above.

* `replacer`, `reviver`

I am less familiar with these so I will have to defer to @josdejong whether they are in the first or second category above. I think they have to do with JSON encoding and decoding.

[josdejong]

Thanks David, this is nice! I made one inline comment.

About the list with failing functions: Glen is right:

• No need for documentation:
  • addScalar, divideScalar, equalScalar, multiplyScalar, subtractScalar, apply
  • replacer, reviver they are indeed for serialization and not used inside the parser itself
• Needs documentation: compile, not, parse, parser, version.

I think the PR is small enough to directly add the missing documentation, I can help with that if needed.

[dvd101x]

Hi, thanks for the review.

Thanks Jos, please help me with the documentation for compile, parse, parser, version

Just a few comments:

• not was failing because I tested help(not) (which throws a syntax error) instead of help("not") in the parser, I will consider that.
• I will include also in the tests math.expression.mathWithTransform
• I will evaluate if the new math instance can be removed as mentioned in the inline comment.

[gwhitney]

• not was failing because I tested help(not) (which throws a syntax error) instead of help("not") in the parser, I will consider that.

I would consider this a bug in the parser, given that help(and) is totally fine. It is a consequence of the fact that not is the only "named" unary operator, and the parser does not contemplate that it might be used as an expression in and of itself, to mean the associated function. So similarly, map([true, false], not) is a syntax error, but I would say that it should be totally valid and return [false, true].

Should I file a separate bug for this? I could address it in #3423, for example. Alternatively, I should also point out that if a proposal along the lines of #3379 were adopted, it would also totally fix this, in that we would no longer need to consider not an operator; it would just be a function, and not x would be interpreted as function application, and it would work as expected. And once not is just a function, help(not) is fine. The precedence of not is also exactly correct for allowing it to be an ordinary function -- right alongside implicit multiplication, which is where this sort of function application would be defined. Frankly, I don't see any reason why not x should be allowed but sin x not be allowed. They actually seem completely parallel to me. I think consistency between not and other unary functions is another strong reason to adopt something like #3379.

[dvd101x]

As a reference, there was some discussion about it at #1905.

[gwhitney]

Ah, that's very helpful. I didn't realize that help(not) throwing had already been definitely classified as a bug. I would be happy to address it. I will continue discussion of it in #1905.

[dvd101x]

Hi, this is ready for review.

I included a new test to validate that the "see also" contains only valid names and doesn't contain the name of the current doc.

[gwhitney]

What was the decision about help(not)? Are we addressing that (see #1905) before this PR, or somehow in this PR, or special-casing it for the tests in this PR and just deferring #1905 at the moment? Thanks for reminding me.

[dvd101x]

While working on it I found a similar case with help(version) so I opted to use double quotes for everything in this PR. Thus help("not")instead of help(not) and help("version") instead of help(version)

At this point I'm not addressing either case, just using double quotes.

[gwhitney]

Functionality looks good, comments mostly on docs and comments

[dvd101x]

Hi Glen, thanks for your review.

Most descriptions I just copied them from the jsdcos and simplified a bit for the embedded docs. But I agree with your comments so fixed both, the jsdocs and the embedded docs. I think all your comments are addressed.

Also passed some of the expressions from syntax to expression to be more consistent with the jsdocs.

[dvd101x]

Just addressed an edge case for names that should have embedded docs but their examples are expected to fail.

[gwhitney]

Oops, look like I gave you some bad advice -- I guess Set.difference is not yet implemented in some of the Node versions mathjs is still intended to run on. So I guess you'll need to see what is available in Node 18 as far as Sets go and re-evaluate whether, if you limit to that portion of the Set api, it's worth using Sets at all. My apologies -- on most of my projects, we always just run in the latest greatest, but mathjs has broader aims than that.

[gwhitney]

OK, as far as I can see, this is ready to merge. I only didn't because I am unsure whether testing-only PRs get a HISTORY entry. @josdejong please let me know, and/or feel free to merge this yourself.

[dvd101x]

Thanks Glen,

I also have a version ready that uses Set that doesn't use new features. I can't decide between both, but I think both would work. Using arrays and using Sets. It looks like this.

// Add names to the skipDocs array if they are not meant to have embedded docs
const skipDocs = new Set(['import', 'addScalar', 'divideScalar', 'equalScalar', 'multiplyScalar',
  'subtractScalar', 'apply', 'replacer', 'reviver'])

// Add names to skipExamples if their examples in the embedded docs contain acceptable errors
const skipExamples = new Set([])

const allTestDocs = new Set([
  ...Object.keys(embeddedDocs),
  ...Object.keys(math.expression.mathWithTransform)
])

const testDocs = new Set()

allTestDocs.forEach(name => {
  if (!skipDocs.has(name)){
    testDocs.add(name)
  }
})

const testExamples = new Set()

testDocs.forEach(name => {
  if (!skipExamples.has(name)){
    testExamples.add(name)
  }
})

[dvd101x]

I have no further changes planned, please ignore my previous comment.

[gwhitney]

All still seems fine, so my last comment stands

[josdejong]

Thanks @dvd101x!

@gwhitney when I just do some refactoring or adding tests, I do not add an entry in the HISTORY.md for that. But if others contribute I find it important to give them credit. And in this case David also improved the embedded docs: added documentation for compile, evaluate, parse, and parser, so that is an actual improvement and not just unit tests :)