Skip to content
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

print is zero indexed in the parser #2989

Closed
dvd101x opened this issue Jun 29, 2023 · 7 comments
Closed

print is zero indexed in the parser #2989

dvd101x opened this issue Jun 29, 2023 · 7 comments

Comments

@dvd101x
Copy link
Collaborator

dvd101x commented Jun 29, 2023

Checking the embedded docs for the function print there is the following example

print("Values: $0, $1, $2", [6, 9, 4])

It's kind of odd since in the parser there is no zero index in matrices, maybe it should be like:

print("Values: $1, $2, $3", [6, 9, 4])

So my suggestion is to review the possibility for print to be one indexed in the parser and adjust it's embedded docs accordingly.

To Reproduce
Write in the parser
help(print)

Try the following example in the parser
print("Values: $0, $1, $2", [6, 9, 4])

@josdejong
Copy link
Owner

Thanks for reporting, we should indeed make the function print one-based in the expression parser, like other functions such as map, subset and index. To do that, we should implement a transform function for it, which changes the indexes used in print from 1-based to 0-based. Anyone interested in implementing that?

@dvd101x
Copy link
Collaborator Author

dvd101x commented Aug 5, 2023

Hi, while working on this and doing tests I noticed that it's expected to work with an array inside an object.

math.print("I like $food.0", {food:["pizza"]})    // "I like pizza"

But in the parser

print("I like $food.0", {food:["pizza"]})         # "I like $food.0"

This seems like a separate issue, shall it be addressed in a separate PR ?

@Tethet-18
Copy link

Thanks for reporting, we should indeed make the function print one-based in the expression parser, like other functions such as map, subset and index. To do that, we should implement a transform function for it, which changes the indexes used in print from 1-based to 0-based. Anyone interested in implementing that?

i would like to try to implement it. Is this issue still open?

@Tethet-18
Copy link

i think i have found the solution to the problem. we just need to alter the file https://github.com/josdejong/mathjs/blob/49c793ba5a47359157a2c5009e63b11e43fcff2e/src/function/string/print.js#L1 and change the indices to the keys-1

@dvd101x
Copy link
Collaborator Author

dvd101x commented Aug 11, 2023

Hi Ayush, I have something kind of ready except for the mentioned issue regarding arrays inside objects. I did a transform function so that the math.print(...) stays zero indexed and math.evaluate("print(...)") is one indexed. That's for one or multiple indexes.

Currently Jos is on holidays so I'm chilling for a bit. Maybe over the weekend I will make a PR if it's useful to you.

@dvd101x
Copy link
Collaborator Author

dvd101x commented Aug 12, 2023

I found the issue regarding arrays inside objects not working in the parser.

The issue is that print doesn't do the automatic conversion from matrix to array if it's part of an object. Thus this also doesn't work.

math.print("I like $food.0", {food: math.matrix(["pizza"])})    // I like $food.0

I think it will be a quick fix and will get it ready in a PR maybe next week.

@josdejong
Copy link
Owner

Fix published now in v11.11.1

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

No branches or pull requests

3 participants