Skip to content

CodeQL: Python topics (2166) - WIP#2823

Merged
felicitymay merged 14 commits intogithub:docs-preparationfrom
felicitymay:2166-python-pre-migration-tasks
Mar 10, 2020
Merged

CodeQL: Python topics (2166) - WIP#2823
felicitymay merged 14 commits intogithub:docs-preparationfrom
felicitymay:2166-python-pre-migration-tasks

Conversation

@felicitymay
Copy link
Copy Markdown
Contributor

@felicitymay felicitymay commented Feb 12, 2020

This PR contains changes that start the work of preparing the CodeQL for Python topics for future migration, as outlined in product-documentation issue 2166.

Notes

For most topics, this PR makes just the following changes:

  • Create an intro.
  • Update some headings for better internal consistency and consistency with content models.
  • Update some links for localization and content model guidelines (ignoring all links to items in the QL library reference which should not be translated).
  • Change "What's next" section into a "Further reading" section.
Original New Notes
Tutorial: Control flow analysis Analyzing control flow in Python In addition to the above, merged an image of a control flow graph into this file (previously in a separate topic).
Tutorial: Functions Functions in Python ---
Introducing the CodeQL libraries for Python CodeQL library for Python ---
Tutorial: Points-to analysis and type inference Pointer analysis and type inference in Python ---
CodeQL for Python CodeQL for Python This map topic now contains only its intro and a toc of the child articles. The other resources are moved into a reusable snippet for use in the "Further reading" section of all child topics.
Tutorial: Statements and expressions Expressions and statements in Python ---
Taint tracking and data flow analysis in Python Analyzing data flow and tracking tainted data in Python ---

Questions

  1. There is a gap between the links in the "Further reading" section that are directly defined and those that are in the resuable snippet. This seems to be unavoidable with Sphinx unless we reverse the order, but the links defined in the topic are more important so I think they should be listed first. I think the gap is acceptable in the short term and better than the alternatives. Happy to discuss if you disagree.
  2. What do you think of this style of intro - review comments very welcome.

@felicitymay
Copy link
Copy Markdown
Contributor Author

A possible flaw with this approach to the introductions - the sidebar has both the new titles and the old titles for the child articles. This may simply be because I haven't updated the titles in the articles yet. I'll look into it on Friday.

@shati-patel
Copy link
Copy Markdown
Contributor

A possible flaw with this approach to the introductions - the sidebar has both the new titles and the old titles for the child articles. This may simply be because I haven't updated the titles in the articles yet. I'll look into it on Friday.

This is probably because the sidebar entries are autogenerated from the elements in toctree (currently using old titles) and the top-level headers in the file (using new titles). Even if you update the titles in the articles, they'll still appear twice!
(I know this Sphinx formatting won't be preserved when we migrate the docs, but if you want things to look nice here you could put the articles back in a list instead of as headers?)

@felicitymay
Copy link
Copy Markdown
Contributor Author

Thanks for the tip Shati - much appreciated.

I've tried the page as a bulleted list and it looks rather cramped to me: http://docteam.internal.semmle.com/felicity/2166-python-bullets/python/ql-for-python.html.
What do you think of: http://docteam.internal.semmle.com/felicity/2166-python/python/ql-for-python.html (which uses a rubric directive)?

@shati-patel
Copy link
Copy Markdown
Contributor

I've tried the page as a bulleted list and it looks rather cramped to me: http://docteam.internal.semmle.com/felicity/2166-python-bullets/python/ql-for-python.html.
What do you think of: http://docteam.internal.semmle.com/felicity/2166-python/python/ql-for-python.html (which uses a rubric directive)?

Agreed, the bulleted list looks pretty cramped, but the second version looks great! 🎉 (Though all of this stuff will be autogenerated once we use the GitHub process, so I guess it doesn't matter too much?)

@felicitymay
Copy link
Copy Markdown
Contributor Author

(Though all of this stuff will be autogenerated once we use the GitHub process, so I guess it doesn't matter too much?)

That's a good point. I think this might be worth talking about. Since the migration isn't expected to start soon, it might be nice to make the pre-migration changes in such a way that we can merge the docs-preparation branch back into master once we've finished the changes. I suspect that we might all have different views on what the resulting output should look like.

@felicitymay felicitymay force-pushed the 2166-python-pre-migration-tasks branch from b0aeeb2 to 7f6e2a4 Compare February 14, 2020 18:04
@felicitymay
Copy link
Copy Markdown
Contributor Author

Following discussions with @shati-patel and @jf205 - I removed the duplicated introductions from the map topic and uncommented the toctree.

@felicitymay felicitymay force-pushed the 2166-python-pre-migration-tasks branch from 7f6e2a4 to 8ab4ceb Compare February 18, 2020 12:19
@felicitymay
Copy link
Copy Markdown
Contributor Author

As discussed, I've backed out the wider-reaching changes that I'd started in the Library introduction topic. I've updated the description to match the commits that are now in this PR and the preview is also updated.

I'd be very grateful for a review.

@felicitymay felicitymay marked this pull request as ready for review February 18, 2020 17:40
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! These topics were definitely due for a tidy-up... I like how you've readjusted the heading levels and added consistent titles for examples 👍

I've made quite a few comments, but most of them are just a single comment that I've duplicated in a few places. Also, feel free to ignore any of my more general comments/questions!

.. toctree::
:glob:
:hidden:
:maxdepth: 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Setting maxdepth to 1 is probably closer to what GitHub map topics look like, but I don't think it really matters! (Admittedly the table of contents would look a bit sparse without the introductions.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree about the maxdepth but thought that, in the absence of introductions on this page, the top-level headings at least gave more information about what was in each topic.

Happy to change to 1 if that's preferred though.

Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Comment thread docs/language/learn-ql/python/pointsto-type-infer.rst Outdated
Comment thread docs/language/learn-ql/python/control-flow.rst Outdated
Comment thread docs/language/learn-ql/python/control-flow.rst Outdated
Comment thread docs/language/learn-ql/python/control-flow.rst Outdated
Comment thread docs/language/learn-ql/python/taint-tracking.rst Outdated
Comment thread docs/language/learn-ql/python/control-flow.rst
The simplest use of the ``ControlFlowNode`` and ``AstNode`` classes is to find unreachable code. There is one ``ControlFlowNode`` per path through any ``AstNode`` and any ``AstNode`` that is unreachable has no paths flowing through it. Therefore, any ``AstNode`` without a corresponding ``ControlFlowNode`` is unreachable.

**Unreachable AST nodes**
Example finding unreachable AST nodes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note for future work: perhaps these two examples could be combined into a single procedural section.

Adding Example to the heading text (here and in other places below) doesn't feel quite right to me, but it's probably fine for this first round of changes (and I can't think of a better suggestion).

===================

This example uses the standard CodeQL class ``Function`` (see :doc:`Introducing the Python libraries <introduce-libraries-python>`).
Functions are key building blocks of Python code bases. You can find functions and identify calls to them using syntactic classes from the standard CodeQL library.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see what you are trying to do here, but I'm not sure the opening sentence Functions are key building blocks of Python code bases. really belongs in an intro as it doesn't really users identify what's in this topic. The second sentence does though ✨

When we start more substantial content updates, I think Functions are key building blocks of Python code bases.... would fit nicely in an opening conceptual section, for example `About functions in Python.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I was trying to differentiate between the style of introductions for referential versus quickstart topics. It felt as if a referential topic ought to have more of a statement and less of a call to action.

However, looking at help.github.com, I can see that two of the three reference topics linked from the content models use "You can" to begin their introductions. https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions is the only exception. So probably this isn't necessary. I can see that you and Alistair have both used the "you can" approach.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've created a Google doc to summarize the questions arising from the pre-migration work we've done so far. I've included some examples and will add this too.

Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Comment thread docs/language/learn-ql/python/control-flow.rst Outdated
Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Many thanks for the review suggestions.

Co-Authored-By: Shati Patel <42641846+shati-patel@users.noreply.github.com>
Comment thread docs/language/learn-ql/python/control-flow.rst Outdated
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! A few more minor things, but this looks good to merge 👍

Comment thread docs/language/learn-ql/python/control-flow.rst Outdated
Comment thread docs/language/learn-ql/python/functions.rst Outdated
Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
@felicitymay felicitymay force-pushed the 2166-python-pre-migration-tasks branch from 1ff1a7e to 7a2bb12 Compare March 2, 2020 15:07
@felicitymay
Copy link
Copy Markdown
Contributor Author

I've updated the introductions following Emily's feedback in https://docs.google.com/document/d/1x917mlEHcpUBQqyKrQ7aogw8dG3FseO32m1s62fuIJA/edit.

I think that this is ready for a final review and merge.

Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
@felicitymay
Copy link
Copy Markdown
Contributor Author

@shati-patel - do you have any more feedback before this is merged?

Comment thread docs/language/learn-ql/python/introduce-libraries-python.rst Outdated
Comment on lines +1 to +2
CodeQL library for Python
=========================
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't match the suggested title in Emily's spreadsheet, but it possibly makes more sense as we only discuss one library in this article. I think we'll need to update the equivalent JS topic, which still uses 'libraries' in the title.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm. This suggests lots of other changes to this topic for consistency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Stick with libraries. We'll update the other language topics accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On balance, the more that I read this topic, the more convinced I am that it ought to be "library" - I think you're right that we should standardize on this, one way or the other. I'm going to leave this as-is for now pending that discussion.

Copy link
Copy Markdown
Contributor

@jf205 jf205 Mar 10, 2020

Choose a reason for hiding this comment

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

I think 'libraries' is consistent with what happens when you include import python in your query--although there is a single import statement, several .qll files (library files) are made available to use in the query. Not a big deal, but I agree we should merge now, standardize later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry - spotted this after I merged it. It's more than just a title though - we are inconsistent within the documentation in how we refer to things. I think of a language library being made up of a series of library files, each of which defines a library module.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems as if I'm probably in a minority.

@jf205
Copy link
Copy Markdown
Contributor

jf205 commented Mar 10, 2020

@felicitymay: As with #2866, I think this is good to merge after you have looked at the most recent suggestions, pending approval from @shati-patel. Further changes can be made in a follow-up PR 👍

@felicitymay
Copy link
Copy Markdown
Contributor Author

Thanks for all the review comments. I'm happy to make any further changes in a follow up PR.

@felicitymay felicitymay merged commit a567dba into github:docs-preparation Mar 10, 2020
@felicitymay felicitymay deleted the 2166-python-pre-migration-tasks branch April 28, 2020 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants