Skip to content

Conversation

shati-patel
Copy link
Contributor

@shati-patel shati-patel commented Jun 15, 2020

Uses the detailed description from the QL spec (#3476) to update the more lightweight description of module resolution in the QL handbook.

@shati-patel shati-patel marked this pull request as ready for review June 15, 2020 07:57
@shati-patel shati-patel requested a review from jf205 as a code owner June 15, 2020 07:57
@shati-patel shati-patel removed the request for review from jf205 June 15, 2020 07:58
@shati-patel
Copy link
Contributor Author

@hmakholm, this description is deliberately not too detailed, but could you verify it's still correct?

@hubwriter
Copy link
Contributor

lgtm

hubwriter
hubwriter previously approved these changes Jun 15, 2020
hmakholm
hmakholm previously approved these changes Jun 15, 2020
Copy link
Contributor

@hmakholm hmakholm left a comment

Choose a reason for hiding this comment

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

Looks fair. My comments are just quibbling and can be ignored if you want.

contain ``<queries language="javascript"/>``.)
#. If that fails, it looks up ``examples/security/MyLibrary.qll`` relative to the query
directory, if any.
The query directory is the first enclosing directory containing a file called ``qlpack.yml``. (Or, in legacy products, a file called ``queries.xml``.)
Copy link
Contributor

Choose a reason for hiding this comment

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

"First" is a bit ambiguous here -- but I'm not sure how to say it better without sounding excessively technical.

#. If no file is found using the above two checks, it looks up ``examples/security/MyLibrary.qll``
relative to each library path entry. The library path depends on the environment where you
#. If the compiler can't find the library file using the above two checks, it looks up ``examples/security/MyLibrary.qll``
relative to each library path entry. The library path depends on the tools you use to
Copy link
Contributor

Choose a reason for hiding this comment

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

Even in an abbreviated description, I think it would be helpful to reveal that this is where the libraryPathDependencies of qlpack.yml comes in. We could still omit the details of how exactly it comes in -- just words that amount to "trouble locating imports? check your libraryPathDependencies".

@shati-patel shati-patel dismissed stale reviews from hmakholm and hubwriter via e69c946 June 15, 2020 14:00
@shati-patel
Copy link
Contributor Author

Thanks @hmakholm - good point about the libraryPathDependencies, I've mentioned those!

Co-authored-by: Henning Makholm <hmakholm@github.com>
@shati-patel
Copy link
Contributor Author

Thanks 😄

@shati-patel shati-patel merged commit 3520f2c into github:master Jun 15, 2020
@shati-patel shati-patel deleted the name-res-114 branch June 15, 2020 14:30
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