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 one based in the parser #3009

Merged
merged 26 commits into from Sep 6, 2023
Merged

Conversation

dvd101x
Copy link
Sponsor Collaborator

@dvd101x dvd101x commented Aug 12, 2023

Hi, this is according to #2989

There is no rush, please review when possible.

  • Added a transform function for print
  • Added tests for print in the parser
  • Changed embedded docs
  • Fixed an issue when an object contains a matrix (instead of an array)

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks David, this looks like a good solution. We indeed need a transform function for print. Your solution looks solid and well tested.

One remark: I would like to extract the duplicated logic from print and printTransform and reuse it, to prevent them getting out of sync some day. Do you see a way to do that?

@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Aug 23, 2023

The shared logic is not much other than the regular expression /\$[\w.]+/g (in fact I had a different one, that later on I replaced by the one in print)

The logic to split the results by . is similar but not the same.

I'm thinking of a few possibilities:

  1. From print export the regex that print.transform can import.
  2. Make a separate file that generates the regex and import that from print and print.transform
  3. Use print to modify the print argument (just an idea that would be difficult to make it work for complex cases) print("Values: $1, $2, $3", ["", "$0","$1", "$2"])
  4. Maybe leave it as is

@josdejong
Copy link
Owner

You're right, only the regex can be reused. I prefer having the regex defined only once. Can you please implement your option (2), make a file src/utils/print.js and add the regex there with a clear name? Thanks!

@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Aug 30, 2023 via email

@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Sep 3, 2023

Hi, I included src/utils/print.js

@josdejong josdejong merged commit c35a801 into josdejong:develop Sep 6, 2023
9 of 10 checks passed
@josdejong
Copy link
Owner

Thanks David!

@dvd101x dvd101x deleted the print-one-based branch September 8, 2023 15:30
@josdejong
Copy link
Owner

josdejong commented Sep 20, 2023

The fix is published now in v11.11.1, thanks again!

@richard-viney
Copy link

Unfortunately for the app I work on this was a breaking change and because it was included in a patch release and not a major release it went unreviewed in the package update process. We have to do a bunch of remediations as a result, and will be improving our in-house unit testing of Math.js to try and catch other regressions like this that could affect our app in future, but moving forward is there any policy regarding semantic versioning guarantees in the library?

Semver seems to be the intention, but I couldn't find discussion regarding the breaking nature of this change, is that because it was considered to be a bug? Does Math.js allow bugfixes in patch releases to cause breaking changes to long-standing behaviour, or was it just missed in this instance?

Btw the docs still refer to zero-based indexing here: https://mathjs.org/docs/reference/functions/print.html

Thanks!

@josdejong
Copy link
Owner

I'm sorry to hear that. The library indeed using Semver, and bug fix releases and feature releases should not be breaking.

Every bug fix changes the behavior of the library (normally for the better), but in this case we should indeed have realized that people may rely on the (non-intended) zero-based behavior and mark it as a breaking change.

Btw the docs still refer to zero-based indexing here: https://mathjs.org/docs/reference/functions/print.html

The plain functions do use zero-based indexing, and the expression parser uses a one-based version of the function, see docs: https://mathjs.org/docs/expressions/syntax.html#differences-from-javascript

On a side note: in my projects I use exact version numbers so I have full control, and I upgrade them manually once in a while, so I can directly verify that nothing is broken. Even though minor and bug-fix releases should be non breaking, it sometimes happens that there are unforeseen cases.

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.

None yet

3 participants