launchql / pgsql-parser

PostgreSQL Query Parser for Node.js
MIT License
129 stars 23 forks source link

TruncateStmt plus some progress on CTEs #80

Closed acarl005 closed 1 year ago

acarl005 commented 1 year ago

This PR has the following:

The passes test now after adding upstream/truncate.sql into the tests. However, they do not all pass if I add upstream/with.sql, as there are still some issues when creating recursive views for example. I would like to finish getting the tests to pass, but it was difficult to debug b/c when running the kitchen sink tests, the parsed tree for upstream/with.sql is about 30,000 lines of JSON and looking at the diffs in the test assertions was difficult to understand.

As a tangent, I suspect the implementation of RangeVar might need to change as there is a lot of complexity in that method.

acarl005 commented 1 year ago

Re: the debugging situation in the kitchen sink tests, I might suggest splitting the SQL files by ; and parsing/comparing the statements individually to make debugging easier.

pyramation commented 1 year ago

re: PR

So I take it we need to fix a few issues first? Maybe what we can do for now, is split the new fixture that is running two files... and include the statements that pass in the new one.

re: testing

I agree we need to make some aspects better, for now what I do when I'm testing something specific to make debugging easier:

1 First, change the test you're focused on

- it('does something' () =>
+ it.only('does something' () =>

2 Second, comment out any lines of SQL in the fixtures that you're not testing or worried about, so you can focus on the single query that you're working on.

3 Third, run yarn test:watch and hit the u hotkey to temporarily update all the snapshots. This is just so it only focuses on your new statement.

4 Finally, temporarily modify the check method, potentially adding console logs or whatever as needed.

export const check = (file) => {
  const testsql = glob(`${FIXTURE_DIR}/${file}`).map((f) =>
    readFileSync(f).toString()
  )[0];
  const tree = parse(testsql);
  expect(tree).toMatchSnapshot();
  const sql = deparse(tree);
  expect(cleanLines(sql)).toMatchSnapshot();
  expect(cleanTree(parse(sql))).toEqual(cleanTree(tree));
};

This way, you can drill directly into the statement that you want to test. I understand it's not idea currently, so this is the best way to currently tame the kitchen sink tests.

NOTE: finally, don't forget to remove the .only and any logs, etc. You may also need to update any snapshots by hitting the u hotkey when running yarn test:watch

acarl005 commented 1 year ago

@pyramation thanks for the tips! Those will help. As for point 4, what's your opinion on permanently modifying the check method to parse, deparse, and parse each statement one-at-a-time, rather than the whole file? That would allow the developer to see the exact failing statement without needing to modify it. Happy to implement if you're open to that.

pyramation commented 1 year ago

@pyramation thanks for the tips! Those will help. As for point 4, what's your opinion on permanently modifying the check method to parse, deparse, and parse each statement one-at-a-time, rather than the whole file? That would allow the developer to see the exact failing statement without needing to modify it. Happy to implement if you're open to that.

yea I think if there is an improvement in the developer experience, we should do it. I'm not exactly sure how it would be implemented, but if you have a clear idea I'd suggest we try!

acarl005 commented 1 year ago

@pyramation Published #81 as a followup to this.

pyramation commented 1 year ago

awesome! that PR is awesome. Seems that this one will need some reworking, but at least will be easier to debug now!

acarl005 commented 1 year ago

Closing this PR in favor of #84 which supersedes this.