Skip to content

fix docs egs#535

Merged
LarsAsplund merged 3 commits intoVUnit:masterfrom
dbhi:fix-docs-egs
Sep 10, 2019
Merged

fix docs egs#535
LarsAsplund merged 3 commits intoVUnit:masterfrom
dbhi:fix-docs-egs

Conversation

@umarcor
Copy link
Member

@umarcor umarcor commented Sep 7, 2019

This PR ensures that VUnit.from_argv() is not executed when run.py scripts are sourced to generate the docs (examples). Moreover, function docs_utils is fixed to ignore subdirs of examples/**/ that do not have a run-py file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What concerns me is that these files are starting to address other things than the essentials for the normal user. It's no longer "minimal".

Copy link
Member Author

@umarcor umarcor Sep 8, 2019

Choose a reason for hiding this comment

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

I do understand your concern, but I believe that this PR does not add any complexity. This is because I added absolutely no content. I just indented everything, except the doc block (header), because currently if __name__ == '__main__': does only "protect" ui.main().

From the point of view of a normal user, the single thing that added complexity (if __name__ == '__main__':), was done previously (#516). Nonetheless, I think that it is worth that users need to know this specific command because:

  • It allows to have the documentation of each example included in the sources of the example. I.e., this makes each example subdir atomic, which allows users to move them around without having to track where the docs are located.
    • For this same reason, it also makes it easier to contribute new examples.
  • Although I saw it in the codebase, I did not understand what this __main__ thing did until I had to use it. I think it is good to expose users who are new to Python to this specific feature.

Some further insight about why I contributed this patch now:

Currently, this is not required because none of the commands in the run.py files require a simulator, except ui.main() (and this is protected already). As a result, it is possible to generate the docs in some environment where no simulator is available (i.e. a container with Python and Sphinx only).

However, when 'external modes' are added (#507), ui = VUnit.from_argv() will check the simulator to decide if VHPIDIRECT is supported. As a result, building the docs in the same environment fails, because it does not find any simulator. This is unfortunate, because we want to extract the docstring only; we don't mind about the feature being supported or not.

Although other fixes might be applied (such as setting a dummy ghdl.sh script or actually installing ghdl), I think that this PR is the most idiomatic solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I didn't think about it in #516. Although everyone should learn about if __name__ == '__main__' I'm not sure this is the place. The run documentation doesn't mention anything about this and even if you know what it does it's not clear why it's needed. Remember that this may be the reader's first encounter with Python code.

What you could to is to dynamically create a file starting with def main() and then the rest of the original file indented. If you import that file you can inspect the main function to retrieve the doc string

Copy link
Member Author

@umarcor umarcor Sep 10, 2019

Choose a reason for hiding this comment

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

What you could to is to dynamically create a file

Thanks for the hint! I updated the PR accordingly.

BTW, the following examples do not have a description/docstring: vhdl/coverage, vhdl/third_party_integration, and verilog/verilog_ams.

@LarsAsplund LarsAsplund merged commit 5dd3b8a into VUnit:master Sep 10, 2019
@umarcor umarcor deleted the fix-docs-egs branch September 10, 2019 12:02
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

Comments