Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update deps and adds some useful API documentation #49

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kristianmandrup
Copy link

Please review and merge ;)

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

We're going to need more comprehensive tests for :has. I've left a bunch of comments and will do a follow-up review once they're addressed.

API usage.md Outdated

*Install (via npm for Node.js)*

`npm i esquery --save`
Copy link
Member

Choose a reason for hiding this comment

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

I strongly prefer installation documentation to use npm install .... There's no reason to use shorthand in documentation.

API usage.md Outdated
```js
const esquery = require('esquery');

const conditional = "if (x === 1) { foo(); } else { x = 2; }"
Copy link
Member

Choose a reason for hiding this comment

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

Please use single quotes or double quotes, but not both.

API usage.md Outdated
`[name="x"]:function` - function named `x`
`[name="foo"]:declaration` - declaration named `foo`

## Complex
Copy link
Member

Choose a reason for hiding this comment

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

Combinators

API usage.md Outdated
- adjacent sibling selector (`+`)
- general sibling selector (`~`)

Please see [javascript: expressions-vs-statements](http://www.2ality.com/2012/09/expressions-vs-statements.html)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how this is related. Instead, one should see the estree specification.

Copy link
Member

Choose a reason for hiding this comment

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

You might also want to point them to the CSS documentation because these combinators are both syntactically and semantically identical.

API usage.md Outdated

Please see [javascript: expressions-vs-statements](http://www.2ality.com/2012/09/expressions-vs-statements.html)

`IfStatement > BinaryExpression` - `if` statement followed by a [binary expression](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Expressions_and_Operators) fx `3+4` or `x*y`
Copy link
Member

Choose a reason for hiding this comment

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

No, a BinaryExpression nested directly within an IfStatement.

API usage.md Outdated

`VariableDeclaration ~ IfStatement`

`var` declaration with sibling `if` statement
Copy link
Member

Choose a reason for hiding this comment

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

if statement with sibling var declaration. The if statement is the target.

API usage.md Outdated

### Fields

You can also query on the [Mozilla Parser API](https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Parser_API) fields directly
Copy link
Member

Choose a reason for hiding this comment

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

This link should be updated to estree.

API usage.md Outdated

### Subject

`!IfStatement Identifier` - any not an If statement with an Identifier under, such as `const x = 3` but not `if (x == 2)`
Copy link
Member

Choose a reason for hiding this comment

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

any not an If statement with an Identifier under

any if statement with one or more nested Identifier

API usage.md Outdated

`!IfStatement Identifier` - any not an If statement with an Identifier under, such as `const x = 3` but not `if (x == 2)`

`!* > [name="foo"]` all nodes but those where the immediate child is a node namded `foo`
Copy link
Member

Choose a reason for hiding this comment

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

namded -> named

API usage.md Outdated
- `!:matches(*) > [name="foo"]`
- `!:not(BlockStatement) > [name="foo"]`
- `![left.name="x"][right.value=1]`
- `* !AssignmentExpression`
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a productive example. What is it trying to show off?

Copy link
Author

Choose a reason for hiding this comment

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

Please do whatever it takes to add sufficient docs for how to use the API. I tried my best using the tests cases for inspiration.

@jkjustjoshing
Copy link

@kristianmandrup I hope you update the PR based on the feedback and help get this merged in - I found this document extremely helpful in understanding how to use esquery!

@kristianmandrup
Copy link
Author

Hey! I would be grateful if you would get it merged. Thanks!

@kristianmandrup
Copy link
Author

I made all the fixes as per your suggestions. Please review this PR again and let's make a good API documentation :)

@michaelficarra
Copy link
Member

@kristianmandrup It seems your branch has fallen behind. You should rebase on this repo's master branch. You can also use the GitHub web UI to resolve the conflicts with a commit.

@@ -1,7 +1,7 @@
{
"name": "esquery",
"preferGlobal": false,
"version": "0.4.0",
"version": "0.4.1",
Copy link
Member

Choose a reason for hiding this comment

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

Please don't bump the version. We'll bump it appropriately during a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants