smithy-lang / smithy

Smithy is a protocol-agnostic interface definition language and set of tools for generating clients, servers, and documentation for any programming language.
https://smithy.io
Apache License 2.0
1.7k stars 201 forks source link

Question: turning on warnings as errors? #2303

Closed RenFraser closed 4 weeks ago

RenFraser commented 1 month ago

Hi folks, I'm building my smithy project via gradle and want to turn on warnings as errors.

Is there a recommended way to do this? I've been setting all the validators I can find to DANGER, however this seems unsustainable and overly verbose.

mtdowling commented 4 weeks ago

We don't have plans on adding general purpose elevation like this. You can use severityOverrides if you want to elevate warnings to errors, but you have to do it per/error, and I don't recommend it. The Smithy project, trait vendors, and validation vendors will continue to add new validators in the future that might emit a warning. Failing when encountering an event that isn't designed to fail a build puts a package in a position of blocking progress of the packages it relies on, especially in environments that require a working closure of packages.

RenFraser commented 4 weeks ago

Thanks for the response, Michael! Yes, it would block the build. It's a common pattern in other build systems. Is there a recommended approach here that doesn't use the severity overrides, such as using a linter? I want to prevent the introduction of new issues like adding unused code, using deprecated code etc. Is there a typical approach that people take, like using the linter and extending it to the issues that they want to prevent?

kstich commented 4 weeks ago

Is there a recommended approach here that doesn't use the severity overrides, such as using a linter?

There is not, using the severity overrides is the only way.

Not having a blanket flag gives the flexibility to restrict the situations you know about while allowing vendors to add new validation to guide new models. Otherwise, as @mtdowling said, your package could block progress of those it relies on.

I want to prevent the introduction of new issues like adding unused code, using deprecated code etc.

The exact override is going to depend on what you're trying to prevent, but even these should be judged with high caution. You could upgrade the severity of DeprecatedTrait, but that would mean failing if anything you depend on deprecates a shape or trait you use. If your packages exist in a shared ecosystem, these two items can't be reconciled and you'd block the very valid evolution of your dependency or they would be forced to evict you.

RenFraser commented 4 weeks ago

Thanks @kstich,

but that would mean failing if anything you depend on deprecates a shape or trait you use

Wouldn't scoping validators/overrides/suppressions to namespaces prevent this in a generic way? EG In this example, we'd be able to suppress the third party deprecated warnings and that should not break our build? Maybe for my own learning, what's stopping smithy from achieving this when other compilers can do it? It sounds like it would be a common problem to other build systems too, so I'm wondering why they can do it but not us. The dependency chain problem would still be present in Clang, for instance. Is there a design choice that makes the problem more complex?

kstich commented 4 weeks ago

It's not that we can't, it's that we've chosen not to. This can be a problem with other build systems like Clang. In those language environments, it is recognized as causing pain, not done when building to shared ecosystems, and users are more familiar with the requirements of upgrading.

Doing a lot of work to scope when and how overrides and suppressions are configured just layers complexity on top, so we're forgoing it altogether.

RenFraser commented 4 weeks ago

Fair enough. Thanks for the in depth explanations! I'll close this as resolved :)