Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(cli): Improve CLI documentation structure #265

Closed
wants to merge 0 commits into from

Conversation

santiagogaray
Copy link
Contributor

@santiagogaray santiagogaray commented Mar 15, 2021

Hi @chriswmackey,
Thanks for reviewing and merging all the PRs I sent your way. At this point I was just going down the list of repos we have in the contract and I realized we have some cases (like this repo) without CLI.
I wasn't' sure if we wanted to work on these just yet but then I thought this can be a good case to decide which way (if any) we want these cases to be handled while CLI is not supported.
The way I handled it in this PR was adding a CLI folder and cli\index.rst file with an example message (something like CLI not supported yet.). See example image below.
We could also exclude a cli folder and file but add a CLI Docs section in the main index.rst file with a link to the future cli folder. In this case sphinx will throw a warning that the file can't be found and no link will be displayed, so will render just the CLI Docs title.
In both these cases once the CLI files are created the groups will be found and files will be created as usual. The links and docs will be included. The only text to be added is some installation notes as we usually do in the cli/index.rst file.

So I guess would be good to know if having something like this makes sense at this point or maybe is better to wait till we have CLI on those repos to add these documentation improvements,
Hope all this makes sense Chris. Also, if you rather have me working on other libraries please let me know!
image

@chriswmackey
Copy link
Member

Hey @santiagogaray ,
I am sorry that I was not aware of this PR until today. If you are able to request my review whenever you send a PR, that means that it should show up on my radar sooner than later. I'll try to review and merge this soon.

@chriswmackey chriswmackey self-requested a review March 23, 2021 19:41
@santiagogaray
Copy link
Contributor Author

Hey @chriswmackey ,

Sorry for not requesting your review, I will do so in the future. I just want to let you know though that even then please take your time to get back to me if things are busy on your end. Just making clear I won't expect you to be available each time I send you a review request.
Also Chris, since I sent this PR I decided to take a look again at the other aproach to handling groups with different names from their module. Also I realized that some modules have non group files (such as helper, util) and have to be excluded somewhere in the code or otherwise will show up in the docs as empty CLI module pages. I was feeling uncomfortable with not being able to handle this more reliably in the code and decided to take a stab at the aproach of looking for groups inside the files. I think this is quite simple at the end by using regular expressions. I tested it out on many libraries with different situations, including honeybee-radiance for different group names and others with non group modules and so far seems to be working well.
Please let me know your thoughts, I'm adding the updated conf.py for your review below.

@chriswmackey
Copy link
Member

chriswmackey commented Mar 25, 2021

Hey @santiagogaray ,

Thanks, as always, for putting a lot of thought into this. There is a very real possibility that this repo will get a CLI at some point, but it might be a while before this happens given that it's more likely for the other repos leveraging ladybug-geometry to get commands that might otherwise go here. For example, if I want to rotate a Honeybee model, we would put the command in honeybee-core instead of here (even though ladybug-geometry is doing the heavy lifting).

So the ideal approach would be to put an if statement into the conf.py that could add the whole CLI section if it senses a CLI sub-package or module in the repo. A solution like that could also be implemented on the ladybug-tools-template repo and so that the repos that it sets up can work for both those with and without a CLI. But I realize this may be difficult and, if there's no straightforward way to do it, then what you have here can work. I prefer the message about CLI not available at this time because explicit is better than implicit.

That would be great if you can generate the page names from the CLI groups as opposed to the modules. Like you said, we'll probably have helper modules and things that don't really relate to the commands.

Also, speaking of the other repos, I think there are two repos that might not exist before the end of the contract (dragonfly-radiance and honeybee-iso). But there are two new repos that could really benefit from some docs upgrade:

If you would be open to trading those two items in the scope for these two new ones, it would help us out a lot. Both of them have CLI's that are in need of some better documentation. Let me know what you think.

@chriswmackey
Copy link
Member

Actually, I can see that honeybee-vtk seems to be so new that it's following your recent doc templates. So, if there's nothing to do there, another repo worth considering is the lbt-recipes repo. It currently doesn't have any docs but it would be nice to have docs for the 3 python modules in the root of the package. It's a bit of a weird case as we would probably prefer to not have docs for all of the sub-packages below the root (they are all auto-generated and not really the type of thing that needs SDK documentation). Anyway, let me know what you think.

@chriswmackey
Copy link
Member

Hey @santiagogaray ,
I just wanted to check - are you waiting to update the other repos before coming back to this PR? Or do you think this is fine to merge as-is?
In the end, there's no CLI here yet so I could go either way.

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.

None yet

2 participants