refactor(assert,cbor,collections,fs,toml,uuid): align error messages to the style guide, add error-message Deno Style Guide linter plugin

[timreichen]

Part of #6551

Note: This plugin does not check for 3, 4, 5, 7 and 8 in the Style Guide for error messages as they would need language analysis.

[codecov]

Codecov Report

Attention: Patch coverage is 86.04651% with 12 lines in your changes missing coverage. Please review.

Project coverage is 94.54%. Comparing base (390845e) to head (ed9f2fd).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
fs/_node_fs_file.ts	0.00%	5 Missing ⚠️
_tools/lint_plugin.ts	94.11%	4 Missing ⚠️
cbor/_common_decode.ts	0.00%	2 Missing ⚠️
fs/_get_fs_flag.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6553    +/-   ##
========================================
  Coverage   94.54%   94.54%            
========================================
  Files         580      580            
  Lines       43730    43850   +120     
  Branches     6471     6495    +24     
========================================
+ Hits        41345    41459   +114     
- Misses       2342     2348     +6     
  Partials       43       43            

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kt3k]

Can you exclude the test only error messages from the check target?

[timreichen]

Can you exclude the test only error messages from the check target?

Done. I added single line ignores because most of these tests only cases also should follow the style guide imo but can make the switch at a later point.

[kt3k]

most of these tests only cases also should follow the style guide imo

I disagree with that. That sounds unnecessarily strict rule. Also the style guide says it's rule for User-facing error messages

[timreichen]

most of these tests only cases also should follow the style guide imo

I disagree with that. That sounds unnecessarily strict rule. Also the style guide says it's rule for User-facing error messages

I think it is preferable to have a code base that follows the all the rules whenever possible, also for tests. There are only some exceptions in this particular case of error messages that might be justified to ignore lint rules, like specifically testing lower case errors for example. If the test works the same with a lowercase or uppercase message, there is no reason to not follow the rule imo. Else it is just an abuse of the lint ignore flag, no?

[kt3k]

It makes a contribution barrier and makes it unpleasant to contribute to the project to enforce unnecessary rules. Things like new Error("boom") should be allowed in testing without deno-lint-ignore directive in my opinion.

We can have access to file name via context.filename. I think we can exclude test files using it

[timreichen]

It makes a contribution barrier and makes it unpleasant to contribute to the project to enforce unnecessary rules. Things like new Error("boom") should be allowed in testing without deno-lint-ignore directive in my opinion.

I am not sure which rules are considered necessary and which not but we also enforce other lint rules in test files, so I don't know why we would suspend some and enforce others.

I would not call enforcing lint rules a contribution barrier. A warning can easily be ignored with s comment or auto-fixed.

Wouldn't it be rather confusing if a contributor has to handle different rule sets for an api and tests?

[iuioiua]

I'd lean towards not being overly strict too.

[timreichen]

I'd lean towards not being overly strict too.

But why hardcode ignoring files when one can just add an ignore file comment? This doesn't make sense to me.

[kt3k]

But why hardcode ignoring files when one can just add an ignore file comment?

I don't think we need to hardcode ignore files, but we can detect user-facing errors by using deno_graph + exports fields of each deno.json file.

[iuioiua]

Just nits. Otherwise, LGTM!

[kt3k]

I don't think this should be capitalized as this matches the variable name.

[kt3k]

It looks like we consistently use . in AssertionError message. I think we probably should also exclude AssertionError for the period check.

[kt3k]

LGTM