Skip to content

Conversation

@awakair
Copy link

@awakair awakair commented Oct 17, 2024

Hi! I want to generate docs for proto file which contains thousands of messages and display in index.html only small part of them. But I still want clickable links to packages. This PR will add such functional to your tool


package = packages.get(file.package, Package())
package.name = file.package
package.displayed_name = sable_config.included_to_main_packages.get(file.package, package.name)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this part work? In the next line, we're checking for not sable_config.included_to_main_packages, but here we are using it directly without checking whether this settings is configured or not. Wouldn't this fail if included_to_main_packages was not specified?

@markvincze
Copy link
Owner

@awakair Thanks for the patience on this!

I checked the PR, but it's not clear to me what we're trying to achieve.
In the description you're mentioning that you have thousands of messages, and you want to display only a subset of them.
But in the change we're not limiting the messages we're displaying, but rather the packages, aren't we? (And in the README you use the key MyMessage, but then you search in the dictionary with package.name, right?)
And do I understand correctly that it's also adding the capability to override the name of a package to a different name which will be displayed in the list? This seems like a strange requirement, what is the use case in which you'd want to override a package name?

(Besides, I'm not sure about this implementation, I don't like that we'd be using one config setting for two different purposes, both for controlling which packages are shown in the list, and also overriding the names, I'd rather separate those two capabilities.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants