Skip to content

Conversation

@jbrower2
Copy link
Contributor

Add quotes in example of thousandsSeparator, and fix incorrect output of bytes.parse(1024).

@jbrower2 jbrower2 changed the title Fix two typos Fix two typos in README.md May 29, 2020
@jbrower2 jbrower2 changed the title Fix two typos in README.md Fix two typos in Readme.md May 29, 2020
@dougwilson dougwilson self-assigned this May 29, 2020
Readme.md Outdated

bytes(1024);
// output: 1KB
// output: 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

I am getting 1KB as the output of bytes(1024) in the current module:

$ node -pe 'require("bytes")(1024)'
1KB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @dougwilson yes you're correct. This section of the documentation is about the parse method, though, so perhaps these examples should explicitly read bytes.parse(1024) instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. It looks like when the parse/format were made available, the doc examples were not updated to match. It looks like there is likely a lot to accomplish there, would you be open to splitting that into a separate PR from the first typo? Likely needs to (1) document the bytes function itself with the examples there from both of the two existing sections to demonstrate it, (2) change the examples in format section to be bytes.format and (3) change examples in the parse section to be bytes.parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'd be happy to write that up. Will back out this change for this PR.

@jbrower2
Copy link
Contributor Author

jbrower2 commented Sep 9, 2021

@dougwilson This hasn't been addressed in a little over a year. Are we safe to merge it?

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

apologies i left this to linger.

@dougwilson dougwilson merged commit f78e18a into visionmedia:master Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants