feat(jsii): standardized error messages #1931
Conversation
All errors that jsii can emit now have a dedicated jsii error code (and symbolic name), which will make it easier to track bugs related to those as well as to provide a better experience for the documentation of the errors, their cause, and what users should do to fix them. This lays the foundation for being able to customize the severity of diagnostics (non-error ones, like warnings and suggestions) individually in order to be able to have flexibility with breaking changes.
RomainMuller
added
feature-request
packages/jsii/lib/jsii-diagnostic.ts
Outdated
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/explicit-member-accessibility | ||
| readonly #defaultCategory: ts.DiagnosticCategory; |
There was a problem hiding this comment.
This # is throwing me off and I can't quickly find what it means. Do you have a reference?
There was a problem hiding this comment.
packages/jsii/lib/jsii-diagnostic.ts
Outdated
| * @param location the source code location attachment of the message. | ||
| * @param args the arguments to the message formatter. | ||
| */ | ||
| public create( |
There was a problem hiding this comment.
Why not 2 orthogonal methods?
Code.create(...).attach(...);There was a problem hiding this comment.
I actually want to eventually require all diagnostic messages have a source location attached, so I mean for the node to be provided upfront, always... This is why the other method is deprecated.
packages/jsii/lib/assembler.ts
Outdated
| `Therefore, the module '${assembly.name}' must also be defined under "peerDependencies". ` + | ||
| 'This will be auto-corrected unless --no-fix-peer-dependencies was specified.', | ||
| this._diagnostics.push( | ||
| JsiiDiagnostic.Code.JSII_0005_MISSING_PEER_DEPENDENCY.create( |
There was a problem hiding this comment.
JsiiDiagnostic.Code feels rather verbose. Why not one top-level class? Ultimately we only care about the predefined JSII_0005_MISSING_PEER_DEPENDENCY (et al) constants, no? Why not make them easier to access?
There was a problem hiding this comment.
You're probably right. I've been self-debating this myself, too.
packages/jsii/lib/assembler.ts
Outdated
| isThisType, | ||
| typeUse, | ||
| ).addRelatedInformation( | ||
| JsiiDiagnostic.Code.JSII_9999_RELATED_INFO.create( |
There was a problem hiding this comment.
JSII_9999_RELATED_INFO : this feels like cheating. Are "RelatedInformation"s really a type of Diagnostic?
Or do they just happen to share some features like formatting and a location and was this a convenient way to get that? Some remodeling might be in order.
There was a problem hiding this comment.
The ts.DiagnosticRelatedInformation interface is actually the base interface of ts.Diagnostic (the later only adds properties we don't care about using - reportsUnnecessary, reportsDeprecated, source - as well as the relatedInformation property that is an optional array of ts.DiagnosticRelatedInformation).
The consequence of this is that a code is needed on the related information in the exact same way that for top-level diagnostics... And those codes are actually visible to programmatic users.
|
@RomainMuller What is the convention for the error code numbers? Should we add an explanation for those to the docs? |
…1903) It is illegal in C# to name a class' member (method, property) with the same name as the class itself (even if the member is static). Since both class and member names are pascal-cased in C#, this means a class' members cannot share the same PascalCased representation. This adds a compile-time validation for this condition, effectively calling out member names to have the same PascalCased representation as their declaring class' name. This is currently only a warning, as this is "made to work" by altering the type name by appending `_` at the end of it, which is ugly and dangerous but works, and is currently done in several places). As all warnings, this turns into an error when operating under `--strict`, and future work (i.e: #1931) will allow more granular configuration of these errors, so we can hopefully opt all new codebases into the strict behavior and eventually drop the slugification. Fixes #1880
|
There isn't a general convention at this point. I've laid out some basic categories and allocated sequentially in those... I'd like to document that the error codes are to be handled as experimental at this point and that we may re-allocate them at a later point once we have a better idea of whether there is something meaningful we should use? |
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Merging (with squash)... |
All errors that jsii can emit now have a dedicated jsii error code (and
symbolic name), which will make it easier to track bugs related to those
as well as to provide a better experience for the documentation of the
errors, their cause, and what users should do to fix them.
This lays the foundation for being able to customize the severity of
diagnostics (non-error ones, like warnings and suggestions) individually
in order to be able to have flexibility with breaking changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.