Skip to content

Conversation

SiaraMist
Copy link
Contributor

@SiaraMist SiaraMist commented Dec 2, 2022

For reviewers

  • I followed the strategy laid out by @felicitymay for removing LGTM references.
  • I followed @jf205's recommendations for references to the GitHub Codespaces template.
  • The pull request for the CodeQL Codespaces template README is: Update CodeQL Codespaces template README.md codespaces-codeql#1
  • I renamed the exercises so that linking to the answers would work -- let me know if the names I used are incorrect in any way.

@SiaraMist SiaraMist marked this pull request as ready for review December 9, 2022 00:09
@SiaraMist
Copy link
Contributor Author

@jf205 Here are the updates for the CodeQL docs! There aren't a lot of changes, but I appreciate any feedback you have! 🚀

Copy link
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.

These changes look great. Thanks 💖

I tried to test the tutorials in the codespaces template, but think that I'm missing something because what I saw didn't match up with the the updated README (other PR).

I commented on a couple of small things, some of them problems with the original article.

@@ -17,13 +17,15 @@ QL also supports recursion and aggregates. This allows you to write complex recu
Running a query
---------------

You can try out the following examples and exercises using :ref:`CodeQL for VS Code <codeql-for-visual-studio-code>`, or you can run them in the `query console on LGTM.com <https://lgtm.com/query>`__. Before you can run a query on LGTM.com, you need to select a language and project to query (for these logic examples, any language and project will do).
You can try out the following examples and exercises using :ref:`CodeQL for VS Code <codeql-for-visual-studio-code>`, or you can run them in GitHub Codespaces using the `CodeQL template <https://github.com/codespaces/new?template_repository=github/codeql-codespaces-template>`__. This will open a GitHub Codespaces environment preconfigured to run CodeQL queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that possibly the initial release of the codespaces template is only preconfigured to run these QL tutorials, but might well have misunderstood. It would be great to have @jf205's thoughts on this.

Copy link
Contributor

@jf205 jf205 Dec 9, 2022

Choose a reason for hiding this comment

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

As discussed with @SiaraMist on Slack: I think we should pull the information about the template out of this section into its own 'pull-quote' or similar. This will let us:

  1. make it very clear that it's in beta (so isn't quite GA-worthy yet)
  2. make clear that it's only suitable to do the tutorials (including 'Find the thief' etc.) and a 'traditional' set up should be used to query real code.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to put the content mentioned in https://github.com/github/codeql/pull/11554/files#r1044617689 in a reusable, we could also display it in the language-agnostic tutorials 🤔


Note

For a guided introduction to CodeQL, we've created a CodeQL template (beta) in `GitHub Codespaces <https://github.com/codespaces/new?template_repository=github/codeql-codespaces-template>`__. You can use this template to test CodeQL concepts. However, if you would like to run CodeQL queries on code, you will need to install the CodeQL extension in Visual Studio Code. For instructions, see ":ref:`Setting up CodeQL in Visual Studio Code <setting-up-codeql-in-visual-studio-code>`."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely happy with the phrasing of this note, so if anything comes to your mind that could be improved, I'd be grateful for suggestions! 🙇‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends to some extent what parts you're not happy with. I'll add a suggestion and maybe this will unblock you. Also, if I've understood correctly, they can use the template for everything in this article down to "Example CodeQL queries" and also for the QL tutorials.

Suggested change
For a guided introduction to CodeQL, we've created a CodeQL template (beta) in `GitHub Codespaces <https://github.com/codespaces/new?template_repository=github/codeql-codespaces-template>`__. You can use this template to test CodeQL concepts. However, if you would like to run CodeQL queries on code, you will need to install the CodeQL extension in Visual Studio Code. For instructions, see ":ref:`Setting up CodeQL in Visual Studio Code <setting-up-codeql-in-visual-studio-code>`."
You can use the CodeQL template (beta) in `GitHub Codespaces <https://github.com/codespaces/new?template_repository=github/codeql-codespaces-template>`__ to try out the QL concepts in this article and in the puzzle articles in this folder. The template includes a guided introduction to working with QL, and makes it easy to get started.
When you're ready to run CodeQL queries on actual codebases, you will need to install the CodeQL extension in Visual Studio Code. For instructions, see ":ref:`Setting up CodeQL in Visual Studio Code <setting-up-codeql-in-visual-studio-code>`."

Copy link
Contributor

Choose a reason for hiding this comment

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

Once we've worked out the wording, I think we might want to use this in place of the current reusable about VS Code here: .. include:: ../reusables/setup-to-run-tutorials.rst to this new reusable.

Copy link
Contributor Author

@SiaraMist SiaraMist Dec 13, 2022

Choose a reason for hiding this comment

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

Thank you so much! 💖 I made a couple of tweaks to it in 094a9f4, just to make it fit in other articles too. I also overthought "language-agnostic" vs. "programming-language-agnostic" vs. wondering if that actually makes sense, so let me know if you have thoughts on that. 😅

I also updated the article you linked to include that reusable. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit of a mouthful, but very clear. I'd tend to leave it as is 👍🏻

Thanks for the other updates 💖

@felicitymay
Copy link
Contributor

@SiaraMist - thanks for all the updates. It looks to me as if you've addressed all the feedback here and the article looks great 🎉
@jf205 - do you have any more feedback or is this ready to merge when Siara's online?

@jf205
Copy link
Contributor

jf205 commented Dec 13, 2022

The text looks great ❤️

Before we merge I'm going to rename https://github.com/github/codeql-codespaces-template so that it's in line with the other templates we offer (https://github.com/codespaces/templates). After that we'll need to update the links, which should be the last job before merging.

Copy link
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

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

SiaraMist and others added 2 commits December 13, 2022 09:22
Co-authored-by: James Fletcher <42464962+jf205@users.noreply.github.com>
Co-authored-by: James Fletcher <42464962+jf205@users.noreply.github.com>
@felicitymay
Copy link
Contributor

@SiaraMist - please let me know when you merge this PR and I'll republish the docs to update the CodeQL microsite.

@sabrowning1
Copy link
Contributor

Commenting to keep track of this while Felicity's OOO 🙂

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