search

Technologicat / pyan

Static call graph generator. The official Python 3 version. Development repo.
GNU General Public License v2.0
329 stars 57 forks source link

Filter and html writer #27

Closed jdb78 closed 4 years ago

jdb78 commented 4 years ago

Create a way how to quickly filter pyan callgraphs and export the result as svg or html

Technologicat commented 4 years ago

Nice! Thanks! Review will follow shortly...

jdb78 commented 4 years ago
  • Dropping the 3 in pyan3 is fine, let's do that!

  • Why is the pyan3 script renamed main?

    • IIRC (and please correct me if I'm wrong), this will cause python3 setup.py install to install the script as main in a generic location for binaries (typically /usr/local/bin or ~/.local/bin, depending on whether the --user option was passed to setup.py), which leaves the user with no indication of what the shell command called main does. If so, pyan would be a more descriptive name for the script.

I fixed this by using an entry point in setup.py - somehow never questioned the code I copied somewhere else - sorry!

  • If you want to use static types, that's fine, let's do that. In this case proper type annotations are preferable to documenting static types in the docstring. Python 3.6 is a reasonable base version of the language to require these days.

    • Introducing static types in the main part of the implementation is not a high-priority goal for me, but it doesn't hurt to gradually statically type parts of the codebase.

Now using direct code annotations

  • Does the JavaScript in callgraph.html need to be minified? I would imagine the HTML files generated from this template are at most served locally (with python3 -m http.server), or viewed directly in the web browser (I imagine this shouldn't have CORS issues loading external assets), so clarity is more important than bandwidth. Is a non-minified version easily available? If so, could that be pasted into it instead?

Given that it will be served locally and the HTML is rather short, I believe we do not have to minify (the script inside is already minified and should be directly included in the HTML as browsers prevent loading external sources locally for security reasons). I added a link to the original script for those who want to better understand it.

jdb78 commented 4 years ago

In the future, I would also like to re-format the code with black for consistency and readability

Technologicat commented 4 years ago

Small merge fix for setup.py. I kept the jinja2 requirement from yours, and the pyan3 entrypoint that had arrived in another PR in the meantime.

Technologicat commented 4 years ago

Thanks for making the requested changes! Merged.

Yes, feel free to use black in the future.