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

refactor(archive,cache,datetime,fmt,front-matter): align error messages to the style guide #5706

Merged
merged 11 commits into from
Aug 21, 2024

Conversation

irbull
Copy link
Contributor

@irbull irbull commented Aug 17, 2024

Aligns error messages in archive, cache, front_matter, date_time and fmt folders.

irbull added 3 commits August 17, 2024 16:43
Aligns the error messages in the `front_matter` folder to match the
style guide.

denoland#5574
Aligns the error messages in the `cache` folder to match the style
guide.

denoland#5574
Aligns the error messages in the `archive` folder to match the style
guide.

denoland#5574
Copy link

codecov bot commented Aug 17, 2024

Codecov Report

Attention: Patch coverage is 16.21622% with 31 lines in your changes missing coverage. Please review.

Project coverage is 96.25%. Comparing base (99200dc) to head (18873ed).
Report is 5 commits behind head on main.

Files Patch % Lines
fmt/printf.ts 17.64% 14 Missing ⚠️
archive/tar.ts 11.11% 8 Missing ⚠️
datetime/_date_time_formatter.ts 16.66% 5 Missing ⚠️
archive/untar.ts 0.00% 3 Missing ⚠️
cache/_serialize_arg_list.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5706      +/-   ##
==========================================
- Coverage   96.28%   96.25%   -0.04%     
==========================================
  Files         474      474              
  Lines       38383    38403      +20     
  Branches     5578     5586       +8     
==========================================
+ Hits        36957    36964       +7     
- Misses       1384     1397      +13     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Aligns the error messages in the `datetime` folder to match the style
guide.

denoland#5574
Aligns the error messages in the `fmt` folder to match the style guide.

denoland#5574
@github-actions github-actions bot added the fmt label Aug 18, 2024
@kt3k kt3k changed the title fix: align additional error messages fix(archive,cache,datetime,fmt,front-matter): align additional error messages Aug 19, 2024
fmt/printf.ts Outdated Show resolved Hide resolved
fmt/printf.ts Outdated
@@ -611,7 +621,10 @@ class Printf {
prefix += "0x";
break;
default:
throw new Error("cannot handle base: " + radix);
throw new Error(
"Format number only supports radix 2, 8 and 16: cannot handle radix " +
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest the below

Suggested change
"Format number only supports radix 2, 8 and 16: cannot handle radix " +
`Cannot handle the radix ${radix}. Only 2, 8, 16 are supported`

because of the rule Message should state the action that lead to the error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've merged a slightly different version of this that reads: Cannot handle the radix ${radix}: only 2, 8, 16 are supported

fmt/printf.ts Outdated
@@ -881,7 +898,7 @@ class Printf {
}
default:
throw new Error(
"currently only number and string are implemented for hex",
`Only "number" and "string" are implemented for hex formatting: ${typeof val} is not currently supported`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Only "number" and "string" are implemented for hex formatting: ${typeof val} is not currently supported`,
`Cannot format hex. Only number and string are supported for hex formatting: ${typeof val} is given,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've committed a slightly different version of this that reads: Cannot format hex, only number and string are supported for hex formatting: ${typeof val} is given

archive/untar.ts Outdated Show resolved Hide resolved
archive/untar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
archive/tar.ts Outdated Show resolved Hide resolved
@irbull irbull changed the title fix(archive,cache,datetime,fmt,front-matter): align additional error messages refactor(archive,cache,datetime,fmt,front-matter): align additional error messages Aug 20, 2024
fmt/printf.ts Outdated Show resolved Hide resolved
fmt/printf.ts Outdated Show resolved Hide resolved
Commit suggestions by @kt3k

Co-authored-by: Yoshiya Hinosawa <[email protected]>
fmt/printf.ts Outdated Show resolved Hide resolved
Fixed a syntax error
fmt/printf_test.ts Outdated Show resolved Hide resolved
fmt/printf_test.ts Outdated Show resolved Hide resolved
@iuioiua iuioiua requested a review from kt3k August 20, 2024 23:54
archive/tar.ts Outdated Show resolved Hide resolved
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 changed the title refactor(archive,cache,datetime,fmt,front-matter): align additional error messages refactor(archive,cache,datetime,fmt,front-matter): align error messages to the style guide Aug 21, 2024
@kt3k kt3k merged commit 70011e9 into denoland:main Aug 21, 2024
17 checks passed
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