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.

Sign up for GitHub

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

Jump to bottom

Normalize error message grammar (TS/JS) #25269

Open
irbull opened this issue Aug 28, 2024 · 3 comments
Open

Normalize error message grammar (TS/JS) #25269

irbull opened this issue Aug 28, 2024 · 3 comments
Labels
feat new feature (which has been agreed to/accepted)

Comments

@irbull
Copy link
Contributor

irbull commented Aug 28, 2024

There are a variety of different ways errors in Deno's TS/JS layer are raised. Some are sentence case, others not. Some end with a period, others do not. Some provide the current state of the system, others do not.

Without a standard error guide, it makes it hard for new contributors to determine how to format error message. Furthermore, without a standard error guide, it makes it hard for users to know what to expect.

We just went through the std and normalized the errors there. We should do the same with the JavaScript / TypeScript code in the cli / runtime. There is an issue about improving error messages in the cli [1] which is orthogonal to this.

I think we should start with the Error Guide used in the std and see if that's a good fit for the JavaScript / TypeScript code in the cli [2].

[1] #24699
[2] denoland/std#5574
irbull added a commit to irbull/deno-docs that referenced this issue Aug 28, 2024
@irbull
docs: add error messages to the style guide
734244d 
Adds a section to the style guide that explains how to write error
messages for the TypeScript / JavaScript parts of the Deno codebase.

This style guide was used in the `std` and seems to be a good fit for
the runtime as well. It can be updated as we refactor the existing
error messages.

denoland/deno#25269
@irbull irbull mentioned this issue Aug 28, 2024
Merged
irbull added a commit to irbull/deno-docs that referenced this issue Aug 28, 2024
@irbull
docs: add error messages to the style guide
5a9ec3b 
Adds a section to the style guide that explains how to write error
messages for the TypeScript / JavaScript parts of the Deno codebase.

This style guide was used in the `std` and seems to be a good fit for
the runtime as well. It can be updated as we refactor the existing
error messages.

denoland/deno#25269
@irbull
Copy link
Contributor Author

irbull commented Aug 29, 2024 •
edited
Loading

I wrote a small Deno script to scan the directories of the deno/deno project and determine which folders have JS/TS files and which of those files throw . I excluded /node_modules/, /testdata/, /tests/, ./github/, /bench, and /tsc. Here is what I found:

Folder Throws Status PR
./cli/js 32 ✅ #25406
./ext/broadcast_channel 2 ✅ No obvious problems found
./ext/cache 5 ✅ #25310
./ext/canvas 2 ✅ #25310
./ext/console 3 ✅ #25301
./ext/cron 5 ✅ #25300
./ext/crypto 226 ✅ #25440
./ext/fetch 46 ✅ #25374
./ext/ffi 5 ✅ #25310
./ext/fs 14 ⏳ #25414
./ext/http 37 ✅ #25496
./ext/io 0 ✅ No obvious problems found
./ext/kv 24 ✅ #25500
./ext/net 26 ✅ #25914
./ext/node/polyfills 288 ⏳ #25917
./ext/node/polyfills/_fs 73 ⏳ #25917
./ext/node/polyfills/_process 3 ✅ No obvious problems found
./ext/node/polyfills/_util 20 ⏳ #25917
./ext/node/polyfills/assert 1 ✅ No obvious problems found
./ext/node/polyfills/dns 0 ✅ No obvious problems found
./ext/node/polyfills/fs 0 ✅ No obvious problems found
./ext/node/polyfills/internal 31 ✅ No obvious problems found
./ext/node/polyfills/internal/crypto 91 ⏳ #25917
./ext/node/polyfills/internal/dns 6 ✅ No obvious problems found
./ext/node/polyfills/internal/fs 0 ✅ No obvious problems found
./ext/node/polyfills/internal/process 0 ✅ No obvious problems found
./ext/node/polyfills/internal/test 0 ✅ No obvious problems found
./ext/node/polyfills/internal/util 2 ✅ No obvious problems found
./ext/node/polyfills/internal/util/parse_args 9 ✅ No obvious problems found
./ext/node/polyfills/internal_binding 9 ✅ No obvious problems found
./ext/node/polyfills/path 9 ⏳ #25917
./ext/node/polyfills/readline 0 ✅ No obvious problems found
./ext/node/polyfills/stream 0 ✅ No obvious problems found
./ext/node/polyfills/timers 0 ✅ No obvious problems found
./ext/node/polyfills/util 0 ✅ No obvious problems found
./ext/url 3 ✅ #25914
./ext/web 138 ⏳ #25871
./ext/webgpu 35 ✅ #25719
./ext/webidl 38 ✅ #25625
./ext/websocket 14 ✅ #25622
./ext/websocket/autobahn 0 ✅ No obvious problems found
./ext/webstorage 0 ✅ No obvious problems found
./runtime/examples/extension 0 ✅ No obvious problems found
./runtime/js 46 ✅ #25563
./tools 17 ✅ Folder only contains internal tools
./tools/napi 0 ✅ No obvious problems found
./tools/release 4 ✅ Folder only contains internal tools

Script: https://gist.github.com/irbull/a16232d1a96ee3023501baf48b17a7e9

@oles
Copy link

oles commented Aug 29, 2024

I'd say something like https://github.com/denoland/std/blob/main/.github/CONTRIBUTING.md#error-messages would be a nice addition to the Deno Style Guide!

Good job on the Standard Library, @irbull!

@irbull
Copy link
Contributor Author

irbull commented Aug 29, 2024

Thanks for the kind words @oles. I've proposed a PR to the deno style guide with the same formatting used in the std. denoland/docs#759

I'm putting together a few change-sets that update the JS/TS code in the runtime so we can see how we feel about that here too.

irbull added a commit to irbull/deno that referenced this issue Aug 29, 2024
@irbull
refactor(ext/cron): align error messages
0f2531a 
Aligns the error messages in the cron extension to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Aug 29, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Aug 29, 2024
@irbull
refactor(ext/console): align error messages
574209f 
Aligns the error messages in the console extension to be in-line with
the Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Aug 29, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Aug 30, 2024
@irbull
refactor(ext): align error messages
33b3d58 
Aligns the error messages in the console extension to be in-line with
the Deno style guide. This change-set aligns the messages in:
 - ext/cache
 - ext/canvas
 - ext/ffi

denoland#25269
@irbull irbull mentioned this issue Aug 30, 2024
Merged
marvinhagemeister pushed a commit that referenced this issue Sep 2, 2024
@irbull
refactor(ext/console): align error messages (#25301)
977f813 
Aligns the error messages in the console extension to be in-line with
the Deno style guide.

#25269
irbull added a commit to irbull/deno that referenced this issue Sep 3, 2024
@irbull
refactor(ext/fetch): align error messages
64494ed 
Aligns the error messages in the ext/fetch folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 3, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 3, 2024
@irbull
refactor(cli/js): align error messages
2739b7e 
Aligns the error messages in the cli/js folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 3, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 3, 2024
@irbull
refactor(cli/js): align error messages
0186eb6 
Aligns the error messages in the cli/js folder to be in-line with the
Deno style guide.

denoland#25269
irbull added a commit to irbull/deno that referenced this issue Sep 4, 2024
@irbull
refactor(ext/fs): align error messages
373b21d 
Aligns the error messages in the ext/fs folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 4, 2024
Merged
marvinhagemeister pushed a commit that referenced this issue Sep 4, 2024
@irbull
refactor(ext/fetch): align error messages (#25374)
ce6b675 
Aligns the error messages in the ext/fetch folder to be in-line with the
Deno style guide.

#25269
irbull added a commit to irbull/deno that referenced this issue Sep 5, 2024
@irbull
refactor(ext/crypto): align error messages
bf6194e 
Aligns the error messages in the ext/crypto folder to be in-line with
the Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 5, 2024
Merged
marvinhagemeister pushed a commit that referenced this issue Sep 5, 2024
@irbull
refactor(ext/crypto): align error messages (#25440)
0450c12 
Aligns the error messages in the ext/crypto folder to be in-line with
the Deno style guide.

#25269
marvinhagemeister pushed a commit that referenced this issue Sep 5, 2024
@irbull
refactor(ext/cron): align error messages (#25300)
17b5e98 
Aligns the error messages in the cron extension to be in-line with the
Deno style guide.

#25269
irbull added a commit to irbull/deno that referenced this issue Sep 6, 2024
@irbull
refactor(ext): align error messages
6098b44 
Aligns the error messages in the ext/http and a few messages in the
ext/fetch folder to be in-line with the Deno style guide.

This change-set also removes some unnecessary checks in the 00_serve.ts.
These options were recently removed, so it doesn't make sense to check
for them anymore.

denoland#25269
@irbull irbull mentioned this issue Sep 6, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 7, 2024
@irbull
refactor(ext): align error messages
9043596 
Aligns the error messages in the ext/http and a few messages in the
ext/fetch folder to be in-line with the Deno style guide.

denoland#25269
irbull added a commit to irbull/deno that referenced this issue Sep 7, 2024
@irbull
refactor(ext/kv): align error messages
a0f5a9f 
Aligns the error messages in the ext/kv folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 7, 2024
Merged
@Hajime-san Hajime-san mentioned this issue Sep 8, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 10, 2024
@irbull
refactor(runtime): align error messages
a3bc938 
Aligns the error messages in the runtime folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 10, 2024
Merged
marvinhagemeister pushed a commit that referenced this issue Sep 13, 2024
@irbull
refactor(runtime): align error messages (#25563)
606b7b1 
Aligns the error messages in the runtime folder to be in-line with the
Deno style guide.

#25269
irbull added a commit to irbull/deno that referenced this issue Sep 13, 2024
@irbull
refactor(ext/websocket): align error messages
588512a 
Aligns the error messages in the ext/websocket folder to be in-line with
the Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 13, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 14, 2024
@irbull
refactor(ext/webidl): align error messages
7083aab 
Aligns the error messages in the ext/webidl folder to be in-line with
the Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 14, 2024
Merged
nathanwhit pushed a commit that referenced this issue Sep 17, 2024
@irbull
refactor(ext/websocket): align error messages (#25622)
97b8c9b 
Aligns the error messages in the ext/websocket folder to be in-line with
the Deno style guide.

#25269
nathanwhit pushed a commit that referenced this issue Sep 19, 2024
@irbull
refactor(ext): align error messages (#25496)
282c4c2 
Aligns the error messages in the ext/http and a few messages in the
ext/fetch folder to be in-line with the Deno style guide.

This change-set also removes some unnecessary checks in the 00_serve.ts.
These options were recently removed, so it doesn't make sense to check
for them anymore.

#25269
irbull added a commit to irbull/deno that referenced this issue Sep 19, 2024
@irbull
refactor(ext/webgpu): align error messages
c58c98b 
Aligns the error messages in the ext/webgpu folder to be in-line with
the Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 19, 2024
Merged
marvinhagemeister pushed a commit that referenced this issue Sep 19, 2024
@irbull
refactor(ext/webgpu): align error messages (#25719)
486cb18 
Aligns the error messages in the ext/webgpu folder to be in-line with
the Deno style guide.

#25269
satyarohith pushed a commit that referenced this issue Sep 19, 2024
@irbull
refactor(ext/webidl): align error messages (#25625)
3035cdd 
Towards #25269
satyarohith pushed a commit that referenced this issue Sep 19, 2024
@irbull
refactor(ext/kv): align error messages (#25500)
d420829 
Towards #25269
@lucacasonato lucacasonato added feat new feature (which has been agreed to/accepted) and removed enhancement labels Sep 20, 2024
irbull added a commit to irbull/deno that referenced this issue Sep 25, 2024
@irbull
refactor(ext/web): align error messages
25801fb 
Aligns the error messages in the ext/web folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 25, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 27, 2024
@irbull
refactor(ext): align error messages
0704d61 
Aligns the error messages in the ext folder to be in-line with the Deno
style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 27, 2024
Merged
irbull added a commit to irbull/deno that referenced this issue Sep 27, 2024
@irbull
refactor(ext/node): align error messages
1473d67 
Aligns the error messages in the ext/node folder to be in-line with the
Deno style guide.

denoland#25269
@irbull irbull mentioned this issue Sep 27, 2024
Closed
dsherret pushed a commit that referenced this issue Oct 1, 2024
@irbull
refactor(ext): align error messages (#25914)
41a7089 
Aligns the error messages in the ext folder to be in-line with the Deno
style guide.

#25269

<!--
Before submitting a PR, please read
https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feat new feature (which has been agreed to/accepted)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@irbull @dsherret @oles @lucacasonato