Skip to content

Conversation

orelbn
Copy link
Contributor

@orelbn orelbn commented Mar 28, 2025

Description

Test

  • Tested using existing doc tests

Additional Notes

  • The documentation was mostly generated using AI.
  • I'm not sure if this is the optimal or desired approach to completing this task.

@gwhitney
Copy link
Collaborator

The major difficulty with this PR is that it puts the documentation for all of the newly-documented 'isAccessorNode' etc. functions in the expressions category, rather than the utils category (where I would presume they belong). The AI wasn't quite clever enough to discern how the categorizations work. It also, not unrelatedly, breaks the convention that the markdown/web documentation pages go with the source code for the function, rather than with the embedded docs for accessing within the MaJE (MathJs Expressions) parser. (I do hope that it some future revision of mathjs source, these two things are in the same place! but they are not right now.)

If you would like to correct those things "by hand", a PR along these lines generally would be welcome. I also haven't checked the correctness/quality of the generated documentation -- have you?

Thanks for giving this a shot.

@gwhitney
Copy link
Collaborator

OK, I took a quick look at the generated documentation, enough to know that before this could be merged, someone needs to go over it with a fine-tooth comb. For example, the "Examples" of "isAccessorNode" consists only of math.isAccessorNode(), which is missing its annotation of what it would return (that's why it's not breaking the doc tests; they are just ignoring the example), and which would throw anyway, because it needs an argument. Amusingly the AI made a pretty good example in the embedded docs for this same function isAccessorNode. There are a number of roughly similar issues. I definitely didn't take the time to comb through everything.

@orelbn
Copy link
Contributor Author

orelbn commented Mar 28, 2025

OK, I took a quick look at the generated documentation, enough to know that before this could be merged, someone needs to go over it with a fine-tooth comb. For example, the "Examples" of "isAccessorNode" consists only of math.isAccessorNode(), which is missing its annotation of what it would return (that's why it's not breaking the doc tests; they are just ignoring the example), and which would throw anyway, because it needs an argument. Amusingly the AI made a pretty good example in the embedded docs for this same function isAccessorNode. There are a number of roughly similar issues. I definitely didn't take the time to comb through everything.

Thanks for the initial review!

It's currently a draft because I plan to go through it in detail and fix any issues. I just haven't had the chance to do so yet. Please don't spend your time on that just yet. I mostly wanted to get your opinion on the initial approach.

I had an issue adding the docs to where the function is because I got an error from the docs test.

      AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

[
  'isNumber'
]

should loosely deep-equal

[]
      + expected - actual

      -[
      -  "isNumber"
      -]
      +[]

The type checks functionality doesn't follow the regular convention of other files, as seen with something like isNumeric or isNaN, instead, all of the functions are under /utils/is.js. I assume the error has to do with the way doc generation identifies functions. I am not sure if there is already a set convention for dealing with a file that exports multiple functions. Please let me know if you have an example I can reference.

@gwhitney
Copy link
Collaborator

I don't know that the docgenerator or the doc tests have the ability to handle multiple functions in a source file. I don't think there are any working examples of that. I suspecy that's a big part of why these functions have gone undocumented for so long. I think therefore that a successful PR for this would do one of:

(A) split each exported isBlah function, at least as far as the entity that gets put into the resulting math bundle, into its own source file,
(B) extend the docgenerator and the doc tests to deal with multiple functions in a single file, or
(C) get @josdejong's blessing to, for some functions even though it's not the general convention, have their markdown/web documentation in the embedded docs file as this PR is currently organized, and then tweak the docgenerator to correct the category assignment of such functions.

There might be some other method that is not occurring to me to get this all to work. And personally, I would vote for (A) or (B) to keep things consistent and documentation alongside source code (and hope eventually the embeddedDocs move there too!), but @josdejong might have a different perspective.

@orelbn
Copy link
Contributor Author

orelbn commented Mar 31, 2025

I don't know that the docgenerator or the doc tests have the ability to handle multiple functions in a source file. I don't think there are any working examples of that. I suspecy that's a big part of why these functions have gone undocumented for so long. I think therefore that a successful PR for this would do one of:

(A) split each exported isBlah function, at least as far as the entity that gets put into the resulting math bundle, into its own source file,
(B) extend the docgenerator and the doc tests to deal with multiple functions in a single file, or
(C) get @josdejong's blessing to, for some functions even though it's not the general convention, have their markdown/web documentation in the embedded docs file as this PR is currently organized, and then tweak the docgenerator to correct the category assignment of such functions.

There might be some other method that is not occurring to me to get this all to work. And personally, I would vote for (A) or (B) to keep things consistent and documentation alongside source code (and hope eventually the embeddedDocs move there too!), but @josdejong might have a different perspective.

As for C if it goes against the convention, then I don't think it viable option as it will just create extra tech debt for a future update, and the benefits are not worth the tech debt.

I think A is the easier option and might be easier from a maintability standpoint because adding a lot of documentation is going to make the is.js file fairly large and therefore, more difficult to navigate, but let me know if you prefer one large file over many small files.

@gwhitney
Copy link
Collaborator

Right, I think we've now discussed/progressed as far as we can until @josdejong weighs in.

@josdejong
Copy link
Owner

Thanks for your work Orel, and Glen for reviewing this draft. Sorry for my late reply.

Indeed the code comments should be on top of the actual function implementation. Let's go for option (A): move every isBlah function in a separate source file. We could also look into (B) but I expect will come with some challenges, and also, keeping source files small and contain one thing is a good idea in general.

@orelbn
Copy link
Contributor Author

orelbn commented Apr 23, 2025

reviewing

Sounds good. I will get started on that soon.

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