Skip to content

Conversation

tausbn
Copy link
Contributor

@tausbn tausbn commented Jun 23, 2020

Had this lying around for a while. Rebased it so hopefully none of the tests fail.

@tausbn tausbn marked this pull request as ready for review June 23, 2020 14:18
@tausbn tausbn requested a review from a team as a code owner June 23, 2020 14:18
@tausbn
Copy link
Contributor Author

tausbn commented Jun 24, 2020

Since this is documentation, perhaps @felicitymay would like to take a look? 🙂

tausbn added 6 commits June 24, 2020 21:53
I don't think there's any need to document the parts specific to
metrics or defects, as I don't believe these are used anywhere.
If only we had been _somewhat consistent in how we named the
parameters for these...
I didn't do the `toString` methods in this commit. I'm thinking
they're better to do in a separate commit. (There are 48 undocumented
instances!)
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.

Great to see lots of extra documentation. I've made quite a few suggestions for consistency and a couple for clarity. It's entirely possible that some of my suggestions may be based on a misunderstanding of the intended meaning, or the QL style guide!

Comment on lines +1 to +3
/**
* Provides classes for working with external data.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Having never heard about this before, I have no idea what this is actually about by just reading this comment. Looking through the source code, I can see this is used by the Thrift code. So I guess external data in this context means data that is part of the DB that is not Python code? (or would something like CSV and XML not be a part of this?)

Maybe we can expand this comment, or the one for ExternalData? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be quite honest, I don't know exactly what these classes are used for. That's why I left some of the undocumented. (I can certainly make an educated guess about what they might represent, but I wouldn't feel comfortable making this the official documentation.)

As for the level of documentation, I can only refer you to the JavaScript equivalent: https://github.com/github/codeql/blob/master/javascript/ql/src/external/ExternalArtifact.qll
😉

RasmusWL
RasmusWL previously approved these changes Jun 25, 2020
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

One minor comment I would like to have clarified, but otherwise it looks good to me 👍

really happy to see the parameter names of hasLocationInfo being more readable 💪

Co-authored-by: Felicity Chapman <felicitymay@github.com>
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
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.

Thanks for the explanations and updates.

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Updates looks good to me 👍 🚀

@RasmusWL RasmusWL merged commit 3f0975f into github:master Jun 26, 2020
@tausbn tausbn deleted the python-add-a-bunch-of-documentation branch February 12, 2021 18:03
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