fixed nf to work with negative number

[Akhilbisht798]

Resolves #[Add issue number here]

Changes:

resolve #7046

Screenshots of the change:

PR Checklist

• [x] npm run lint passes
• [ ] Inline documentation is included / updated
• [ ] Unit tests are included / updated

[welcome[bot]]

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

[Akhilbisht798]

hey @perminder-17 I can work on this.

[Akhilbisht798]

[perminder-17]

Thanks, @Akhilbisht798, for your great work so far, and I apologize again for the delay—I hope you don’t mind. I’d like to invite the utility stewards, @limzykenneth, @glopzel, and @davepagurek and @nickmcintyre to share their thoughts as well.

I noticed that @limzykenneth has already opened a discussion on this issue (https://github.com/processing/p5.js/issues/7046#issuecomment-2200266696).

In short, should we be writing let num = nf(-123, 4); where 4 represents the number of digits to include to the left? Currently, the output produced after solving by @Akhilbisht798 is -0123, but if we count the minus sign (-) as a character, the output would be -123. The question is: should we consider the minus sign in the digit count?

From my opinion I go with not counting minus sign(-).

I’d appreciate everyone’s input on this discussion that @limzykenneth started. Also, if you have any other suggestions regarding nf(), please feel free to share. (To be honest, I haven’t used nf() much myself, so I’m a bit uncertain about it.)

[limzykenneth]

I think we can possibly go with not counting the minus sign, which is also the behaviour of Processing. For those who needed the numbers to line up, nfs() can solve that.

[ashish1729]

do we need to update some test + reference ?

[perminder-17]

Hmm..I realize I made an error earlier. When I reviewed the code again, I found that it wasn’t rounding the numbers as I initially thought, and the mistake was on my side. I sincerely apologize for any confusion caused. It seems your previous commit was indeed correct. I hadn’t thoroughly checked the code and was only focusing on the testing, which led me to overlook this. After testing it again now, everything appears to be working perfectly.

here's the build of your previous commit: https://editor.p5js.org/aman12345/sketches/Pq9D8HM0h

We can revert to the previous commit, as it looks all good to me. I’m really sorry for any inconvenience caused, and then we could merge this. Really thankful for your patience and hard work @Akhilbisht798.

[perminder-17]

do we need to update some test + reference ?

Yes, adding tests would be a great idea. I believe we already have a good reference sketch with a detailed explanation in the code. If @Akhilbisht798 has some time, we could also add a test for negative numbers, which is currently causing issues. You can check it here: here: https://github.com/processing/p5.js/blob/7d393ec4215ce20f7bc3178b779e0ec67689f928/test/unit/utilities/string_functions.js#L79

we could probably add the same way as it is written here ;)

[Akhilbisht798]

@perminder-17 It is not a problem. I will revert back to the previous commit and will add the test case for this one also.

[Akhilbisht798]

@perminder-17 all done you can let me known.

[ashish1729]

should update the reference for negative numbers

I can see myself using this function for having a fixed-width display... but the way it is implemented negative numbers have more width due to the extra minus sign.

can be taken up as a separate task to unblock the merging of this PR

[perminder-17]

should update the reference for negative numbers

Yes, we could consider adding reference for negative numbers as well. However, since the issue specifically addresses the bug 'nf() produces problematic string formatting for negative numbers,' this PR resolves that issue correctly. It might be a good idea to handle any additional improvements in a separate task.

[ashish1729]

oh by reference I meant documentation, the reference page of the website ..

[perminder-17]

oh by reference I meant documentation, the reference page of the website ..

Yep, I was imagining the same. Sorry... I will make the above comment more clearer. Haha😅

[ashish1729]

then why is documentation considered additional improvement ? shouldn't it be in sync with any update in the code?

the PR on top has 3 checkboxes, which is included in every PR

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

It was my understanding that documentation should be updated to check the 2nd point

[perminder-17]

That's a good point. However, since the PR has been open for five months and @Akhilbisht798 has been very patient and kind in making updates and adding tests, requesting more changes one by one might not be ideal. @Akhilbisht798, if you're interested, it would be great if you could add the documentation for negative numbers, but if you're busy, I'd be happy to work on it and submit a PR. Again thanks a lot for your hard work and patience.

[ashish1729]

I am also trying to learn how thing works, so thank you for explaining.

[Akhilbisht798]

@perminder-17 No problem, I can add documentation changes. but I personally think that the inline documentation for this function is fine and explain things perfectly and to just a changes for negative number won't be that big of deal and won't be changing the functionality in a big way

[perminder-17]

Right, the current documentation is also very good no doubt. But as @ashish1729 said,

I can see myself using this function for having a fixed-width display... but the way it is implemented negative numbers have more width due to the extra minus sign.

Also, one more reason could be, we are not counting the minus sign, so we could also add a line of comment describing it in the code with an example sketch where it shows a negative number.

What you think?

[Akhilbisht798]

ya no problem i can do that.

[perminder-17]

currently when I run grunt yui:dev it shows me this alignment. Can we fix it? also Can we add the negitve number in the describe() function? Let me know if that works

[Akhilbisht798]

hey @perminder-17 let me known