P5.js: nfs() doesn't add "_" (space) in front of 0

[Nekzuris]

Nature of issue?

• [x] Found a bug
• [ ] Existing feature enhancement
• [ ] New feature request

Most appropriate sub-area of p5.js?

• [ ] Color
• [ ] Core/Environment/Rendering
• [ ] Data
• [ ] Events
• [ ] Image
• [ ] IO
• [ ] Math
• [ ] Typography
• [ ] Utilities
• [ ] WebGL
• [x] String Functions: nfs()

Which platform were you using when you encountered this?

• [ ] Mobile/Tablet (touch devices)
• [x] Desktop/Laptop
• [ ] Others (specify if possible)

Details about the bug:

The nfs() function doens't add a space in front of the value 0.
current behavior:
nfs(0, 1, 2) = "0.00"
instead of:
nfs(0, 1, 2) = "_0.00"
( "_" represent a space in the above examples)

• p5.js version: 0.8.0
• Web browser and version: Firefox 66.0.5
• Operating System: Windows 10
• Steps to reproduce this:
  try nfs(0, 1, 2)
  

[welcome[bot]]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

[stalgiag]

Thanks for the issue submission!

This is a bit ambiguous. The documentation says this method "puts an additional "_" (space) in front of positive numbers" but this check will ignore 0 which isn't positive or negative. If we want to allow this to put a space in front of 0 we could do this instead:

function addNfs(num) {
  return Math.sign(parseFloat(num)) > -1
    ? ' ' + num.toString()
    : num.toString();
}

I think this is what most people will expect. Maybe then the docs should change to saying 'non-negative' instead of 'positive', not sure.

[stalgiag]

Or parseFloat(num) >= 0 🤣

[Nekzuris]

Or parseFloat(num) >= 0

Yeah that seems better ;)
I really think this should be the correct behavior of nfs(), there is no reason to exclude zero here.

[stalgiag]

I agree. Any interest in opening a pull request?

[Nekzuris]

Yes, should I do it? I'm just getting into programming, never use github before, not really sure how to do it.

[stalgiag]

That seems like a good time to give it a try to me. But also no pressure! This isn't high priority so time can be taken with it.

If you do decide to give it a go. This can be a useful resource.. After you figure out how to build the library in a local copy of your fork (ie run npm run grunt and see all of the tests pass etc) then you can make a new branch where you just change this line to:

return parseFloat(num) >= 0 ? ' ' + num.toString() : num.toString();

and in the documentation in this line you can change "positive" to "non-negative". Then if you can still run npm run grunt successfully you are free to commit those two changes, push to your fork, and open a PR here with your branch.

Also, if you do decide to give it a go and you need help feel free to ping me with @stalgiag. You can also do that if you are sure that you don't want to do it, and I can add the "help wanted" tag to this issue to enlist help from someone else.

[Nekzuris]

Thanks for your help, I will give it a try ;-)

[Nekzuris]

@stalgiag I struggle to build from the source with npm run grunt, I'm getting these errors:

Aborted due to warnings.

npm ERR! Linux 4.18.0-14-generic
npm ERR! argv "/usr/bin/node" "/usr/bin/npm" "run" "grunt"
npm ERR! node v8.10.0
npm ERR! npm  v3.5.2
npm ERR! code ELIFECYCLE
npm ERR! [email protected] grunt: `grunt`
npm ERR! Exit status 3
npm ERR! 
npm ERR! Failed at the [email protected] grunt script 'grunt'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the p5 package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     grunt
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs p5
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls p5
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /home/louis/Documents/Code/p5.js/npm-debug.log

npm-debug.log

[Spongman]

Try doing an npm install first.

[Nekzuris]

I did, thought there where a few warnings but no error.

[Nekzuris]

I've re-clone the repository and when I run npm install I get these warning:

log.txt

[stalgiag]

Hm I am running a more recent node/npm setup but I don't know if that would be it. Updating might be worth a try if you are in a position to do so, otherwise I can investigate a bit later. Also we can wait to see if anyone has had a similar problem before.

[Nekzuris]

Yes! it work with node v10.15 and npm v6.4.1 but I've got this after npm install

added 1323 packages from 1487 contributors and audited 7120 packages in 61.467s
found 30 vulnerabilities (3 low, 18 moderate, 9 high)
  run `npm audit fix` to fix them, or `npm audit` for details

[Spongman]

that's fine, you can ignore those.

[Nekzuris]

So I've made the changes, run grunt, commit but I'm unable to push, did you know why?

[00:46:58]louis@Lenovo-ideapad:p5.js$ ssh -T [email protected]
Hi Nekzuris! You've successfully authenticated, but GitHub does not provide shell access.
[00:48:05]louis@Lenovo-ideapad:p5.js$ git push origin fix-nfs
Username for 'https://github.com': Nekzuris
Password for 'https://[email protected]': 
remote: Permission to processing/p5.js.git denied to Nekzuris.
fatal: unable to access 'https://github.com/processing/p5.js.git/': The requested URL returned error: 403

[stalgiag]

Maybe you aren't working off of your fork? I am not sure but it looks like you are somehow trying to push to origin which is processing/p5.js (which should be upstream) but you need to push to nekzuris/p5.js

your origin should be nekzuris/p5.js and
your upstream should be processing/p5.js

if that isn't the case then you are probably not working within a copy of your fork.

[Nekzuris]

Ok so it looks like I've made my first pull request :smiley:

Thanks you @stalgiag and @Spongman :+1:

[stalgiag]

closed via #3747