markvincze / sabledocs

Simple static documentation generator for Protobuf and gRPC contracts.
MIT License
59 stars 14 forks source link

allow the "Default Value" column to be turned off #37

Closed wjackson closed 1 year ago

wjackson commented 1 year ago

In the rendered docs that I've seen the "Default Value" column for messages is always empty. This PR adds a config field show-default-value that lets you disable that column.

markvincze commented 1 year ago

@wjackson I'm a bit on the fence about this one. Eventually I'd be happy if there were multiple different templates, or just a more streamlined support for creating custom templates. Thus I would not like to couple the basic data model and the SableConfig to the way the default template works. (Of course probably it will be influenced to some extent by the default template, as that's what we use during the development of the parser, but still I'd like to keep it as generic as possible.) So I'm not sure I want to introduce a show-default-value config value, which might not be relevant in other templates.

What I'd advise doing (which was briefly discussed here too: https://github.com/markvincze/sabledocs/pull/13), is to introduce a computed property on the Message type, which returns whether there is at least one field with a default value, something like this:

    @property
    def has_any_fields_with_default_value(self):
        return any(filter(lambda f: f.default_value, self.fields))

And then use this as the condition in message.html to decide whether to include the column or not. As default values are not even supported in Proto3, I think this would already hide the column for every message in a huge portion of projects. And in Proto2 modules where there actually are default values in a message, it's probably not a good idea to hide them.

What do you think? Would this work for your use case?

wjackson commented 1 year ago

What do you think? Would this work for your use case?

Yep. And I agree that your suggestion seems like a better approach. I'll close this PR then.