DataDog / datadog-static-analyzer

Datadog Static Analyzer
https://docs.datadoghq.com/static_analysis/
Apache License 2.0
100 stars 12 forks source link

StaticAnalysisConfigFile: do not error if the configuration is empty #436

Closed albertvaka closed 3 months ago

albertvaka commented 3 months ago

What problem are you trying to solve?

Do not error if the user deletes everything in the config file, then tries to onboard from scratch again. The previous code would fail because an empty string is technically invalid YAML.

What is your solution?

When we see an empty string as the config, assume there's no config.

What the reviewer should know

This code is used from the IDE-plugin-facing API to modify the config file.

albertvaka commented 3 months ago

@robertohuertasm thanks for the tips. I've pushed some changes. One question, though: did I really need all of Debug, Default, Clone and PartialEq, or only Default was needed and the rest you added because it's good to always have them?

robertohuertasm commented 3 months ago

Regarding your question, the only "needed" one was PartialEq as it was helpful for the test assertion. The other ones were more a nice-to-have. Default was convenient to write less. Debug and Clone are not used, but it's nice to have them implemented.

It is a good practice to implement a few traits when you're dealing with public APIs to make your structs much more versatile and easier to work with:

In our case, this struct is not strictly part of a public facing library so we can/could omit some implementations. The suggestions were indeed just a way to start a discussion about this, not really critical changes.