Skip to content

CodeQL migration: Java topics - change titles & add intros (2164)#2866

Merged
hubwriter merged 4 commits intogithub:docs-preparationfrom
hubwriter:alistairs-docs-preparation-1
Mar 10, 2020
Merged

CodeQL migration: Java topics - change titles & add intros (2164)#2866
hubwriter merged 4 commits intogithub:docs-preparationfrom
hubwriter:alistairs-docs-preparation-1

Conversation

@hubwriter
Copy link
Copy Markdown
Contributor

@hubwriter hubwriter commented Feb 18, 2020

product-documentation issue #2164

Summary

This changes the titles of the Java topics in the "Learning" section, and adds intros as the first paragraph.

I've also made some small editorial changes to bring the style a little more in line with GitHub's writing style. However, I haven't made any larger content changes or structural changes.

Changes in this PR

Article preview Source Notes
"CodeQL for Java" Source
"CodeQL library for Java" Source
"Analyzing data flow in Java" Source
"Types in Java" Source I think "Java types" would be a better title. "Types in Java" sounds like this article is explaining types in Java, when in fact it is explaining how to use CodeQL to write queries that investigate the use of particular data types in Java code.
"Expressions and statements in Java" Source The title is not good. This topic is about writing queries to identify comparisons between different data types where one side of the comparison might overflow. The title is misleadingly general. It's quite a specific little topic. I'd suggest something like: Overflow-prone comparisons in Java
"Navigating the call graph" Source
"Annotations in Java" Source
"Javadoc" Source
"Working with source locations" Source
"Abstract syntax tree classes in Java" Source I think the inclusion of "Abstract syntax tree" in this heading is unnecessary. This reference topic does not mention the AST anywhere. I think "CodeQL classes for Java" might be a better title. I'm assuming that these classes are CodeQL classes and not Java classes (which "classes in Java" leads you to think). Alternatively, add some content near the start of the topic mentioning the AST and its nodes.

Points needing attention

  • Some topics contain links to queries in the query console of LGTM.com. This is done using the linked text "See this in the query console." Reading this as a newcomer this seems strange with no context, and it's going to seem stranger once this is in GitHub documentation as it takes you to a website that readers may not have heard of and, currently at least, seems to have no connection with GitHub. If these links are to stay then I think the text needs to say "See this in the query console on LGTM.com."
  • I've changed "What next?" at the end of some topics to "Further reading". However, I haven't altered the content of these sections, which is discursive, rather than the GitHub style of just having links (one linked article per bullet point). These probably need redone in GitHub style, but some probably need pruned down to avoid too long a bullet list.
  • I've removed a few instances of "thus", which is overly stuffy/formal (if not archaic) and doesn't belong in software documentation. We should remove this word throughout (it only appears in one existing GitHub article).

@ghost
Copy link
Copy Markdown

ghost commented Feb 18, 2020

CLA assistant check
All committers have signed the CLA.

@hubwriter hubwriter marked this pull request as ready for review February 19, 2020 12:56
@felicitymay
Copy link
Copy Markdown
Contributor

Some topics contain links to queries in the query console of LGTM.com. This is done using the linked text "See this in the query console." Reading this as a newcomer this seems strange with no context, and it's going to seem stranger once this is in GitHub documentation as it takes you to a website that readers may not have heard of and, currently at least, seems to have no connection with GitHub. If these links are to stay then I think the text needs to say "See this in the query console on LGTM.com."

That's a good point. When the link text was written, this documentation was embedded in LGTM. Clearly we should have thought to update the text when we moved the documentation out of LGTM. Given that this is used across the entire set of documents, please can you raise an issue. It sounds like we need to decide whether to keep these links and, if so, what the new link text should be.

@felicitymay
Copy link
Copy Markdown
Contributor

felicitymay commented Feb 19, 2020

Your comments on the "Types in Java" and "Expressions and statements in Java" titles - this is a good point and probably also applies to similar topics/titles in Python. Possibly, once we've discussed the content type of these topics with Emily, it may be easier to define a more descriptive title.

@felicitymay
Copy link
Copy Markdown
Contributor

Title: Abstract syntax tree classes in Java

What you say about this topic makes sense. Based on the information in the library overview section "Abstract syntax tree", it sounds as if your second suggestion might be the right answer. That is, add a short conceptual section explaining that these are the CodeQL classes used to model Java statements and expressions.

Copy link
Copy Markdown
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Your "small editorial changes" look good to me. It's great to see some of the old passive voice disappearing 😃

A couple of very minor comments. Otherwise the only thing I wanted to mention was that the link text will need to be manually updated at some point, unfortunately Sphinx doesn't handle this 😞


Overview
--------
When you need to analyze a Java program, you can make use of the large collection of classes in the Java library for CodeQL.
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.

Not entire sure about using "need" here, perhaps "want" or simply "When you analyze..."?


Overview
--------
When you need to analyze a Java program, you can make use of the large collection of classes in the Java library for CodeQL.
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.

Java library for CodeQL

I think this needs to be "CodeQL library for Java" because the library is written in CodeQL.

--------
When you need to analyze a Java program, you can make use of the large collection of classes in the Java library for CodeQL.

About the Java 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.

While it's long winded, I think this also needs to be "the CodeQL library for Java".

For dealing with generic methods, there are classes ``GenericMethod``, ``ParameterizedMethod`` and ``RawMethod``, which are entirely analogous to the like-named classes for representing generic types.

More information on working with types can be found in the :doc:`tutorial on types and the class hierarchy <types-class-hierarchy>`.
For more information on working with types, see the :doc:`tutorial on types and the class hierarchy <types-class-hierarchy>`.
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.

Since this topic is no longer called a tutorial, this probably needs to be updated. This is likely to affect other links too.


Overview
--------
You can use the location of entities within Java code to look for potential errors. Locations allow you to deduce the presence, or absence, of white space which, in some cases, may indicate a problem.
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.

The second sentence could perhaps be simplified?

Suggested change
You can use the location of entities within Java code to look for potential errors. Locations allow you to deduce the presence, or absence, of white space which, in some cases, may indicate a problem.
You can use the location of entities within Java code to look for potential errors. Locations allow you to deduce how white space is used which, in some cases, may indicate a problem.

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 think the longer version is clearer.

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 improving these topics @hubwriter! This looks good already.

I agree with your comments about the titles. The "AST class reference" is basically a summary of CodeQL classes that let you work with Java statements and expressions. Also, I think your suggestion of "Overflow-prone comparisons in Java" is a much better title than "Expressions and statements" 💯

(And thanks for removing "thus"! 🙈)

Navigating the call graph
=========================

CodeQL provides an API for identifying code that calls other code, and code that can be called from elsewhere. This allows you to find, for example, methods that are never used.
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.

Not something you introduced, but it's a bit odd to talk about an "API" here instead of "library". The CodeQL libraries work as an API for interacting with code, but we don't use this terminology much in the docs. (And in fact we should probably get rid of it entirely to avoid confusion.) The classes/predicates for navigating the call graph are part of the CodeQL library (just like the other classes for working with annotations, locations, Javadoc...)

Perhaps remove "API" from the intro at least? (and possibly from the header and example below - but we can deal with those other mentions separately, including in some other topics: "Location API", "Javadoc API" etc)

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.

done

@hubwriter
Copy link
Copy Markdown
Contributor Author

Created an issue to discuss/decide about text of links to the query console:
https://github.com/github/product-documentation/issues/2237

@jf205
Copy link
Copy Markdown
Contributor

jf205 commented Mar 10, 2020

@hubwriter this looks pretty good to me. I'd be in favour of merging this (pending approval from @shati-patel and @felicitymay), with a view to having another pass through when the other 'CodeQL for X' PRs have all been completed. What do you think?

@hubwriter
Copy link
Copy Markdown
Contributor Author

@jf205 - thanks, I'm happy to merge this whenever.
@felicitymay was worried that there would be problems because I merged master into this branch, so I've held off merging. I can easily remove that merge though.

@jf205
Copy link
Copy Markdown
Contributor

jf205 commented Mar 10, 2020

@felicitymay was worried that there would be problems because I merged master into this branch

Fair point. Looks like you could just drop that commit before you merge.

@jf205
Copy link
Copy Markdown
Contributor

jf205 commented Mar 10, 2020

@hubwriter: #3035 is now merged, so it looks like you can cleanly merge this PR without dropping Merge branch 'master' into alistairs-docs-preparation-1 (fe9f974)

@hubwriter
Copy link
Copy Markdown
Contributor Author

Merging

@hubwriter hubwriter merged commit 44b9773 into github:docs-preparation Mar 10, 2020
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.

4 participants