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

BREAKING(fmt/bytes): rename prettyBytes to format #2896

Merged
merged 6 commits into from Nov 18, 2022
Merged

BREAKING(fmt/bytes): rename prettyBytes to format #2896

merged 6 commits into from Nov 18, 2022

Conversation

timreichen
Copy link
Contributor

@timreichen timreichen commented Nov 16, 2022

  • rename prettyBytes to format
  • deprecate prettyBytes
  • rename PrettyBytesOptions to FormatOptions
  • deprecate PrettyBytesOptions

This is a change in an effort to have the two std formatting mods fmt/bytes and fmt/duration (#2871) have similar naming schemes inspired by existing web apis (https://github.com/tc39/proposal-intl-duration-format).

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

The code changes LGTM minus my comment. @kt3k is this a name change that we want to make?

fmt/bytes.ts Outdated
@@ -17,7 +17,7 @@ type LocaleOptions = {
};

/**
* The options for pretty printing the byte numbers.
* @depreacted (will be removed after 0.170.0) use `FormatOptions` instead
Copy link
Contributor

Choose a reason for hiding this comment

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

This is spelled incorrectly in a few places:

Suggested change
* @depreacted (will be removed after 0.170.0) use `FormatOptions` instead
* @deprecated (will be removed after 0.170.0) use `FormatOptions` instead

@kt3k
Copy link
Member

kt3k commented Nov 16, 2022

@cjihrig

@kt3k is this a name change that we want to make?

I'm not sure. I want to hear more opinions from the community and core members.

Maybe let's keep this PR open for a while?

@MierenManz
Copy link
Contributor

The rename seems like a good idea to me. Consistency is best

@kt3k kt3k changed the title (fmt/bytes): rename prettyBytes BREAKING(fmt/bytes): rename prettyBytes to format Nov 17, 2022
@retraigo
Copy link
Contributor

The renaming would make the fmt module itself consistent if #2871 is going to be merged. It will be weird if fmt/duration is named format and fmt/bytes is named prettyBytes while they both do similar stuff.

@kt3k
Copy link
Member

kt3k commented Nov 18, 2022

I'm now slightly in favor of this change as 'format' sounds like a more appropriate name for standard modules than 'prettyBytes'.

@lino-levan
Copy link
Contributor

+1 on the name change, feels a lot more "standard module" and it has the bonus of being closer to web naming specs.

@kt3k
Copy link
Member

kt3k commented Nov 18, 2022

Looks like we are all supportive to this.

@timreichen Thanks for your suggestion!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit 3832a1f into denoland:main Nov 18, 2022
@timreichen timreichen deleted the fmt_bytes branch November 18, 2022 14:54
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

6 participants