Skip to content

Conversation

@jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Apr 18, 2023

resolves #6671

Description

I'd like to remove these events! They're noisy, they add a lot of clutter to debug-level logs, and I don't think they're particularly helpful in debugging (even while debugging parsing errors) — certainly not as much as doing a better job of tracing parsing errors to specific lines/files, or try-catching to handle them more gracefully.

$ dbt --debug --no-partial-parse parse
10:31:23  Running with dbt=1.5.0-rc1
...
10:31:24  Partial parsing not enabled
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1603: static parser failed on ...
10:31:28  1602: parser fallback to jinja rendering on ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...
10:31:28  1699: static parser successfully parsed ...

As it turns out, MacroFileParse and GenericTestFileParse have already been effectively disabled in v1.5, because they were placed under an if get_flags().MACRO_DEBUGGING (??) conditional.

For the sake of test_all_experimental_parser, we still need to fire the Note events during model parsing, but only when running tests. I switched these out for fire_event_if, and borrowed this bit of logic from the event system as the conditional. I'm happy to rework this in a way that's less repetitive.

Honestly, the functional test itself could probably use a refactor, but I didn't see a quick way to go about that within the scope of this (much smaller) issue.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • Docs changes are not required/relevant for this PR
  • I have run changie new to create a changelog entry

@jtcohen6 jtcohen6 requested review from a team as code owners April 18, 2023 10:13
@cla-bot cla-bot bot added the cla:yes label Apr 18, 2023
@jtcohen6 jtcohen6 force-pushed the jerco/6671-rm-parse-events branch from 9e84bc6 to 0234c54 Compare April 18, 2023 10:28
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

We might want a 'fire_event_if_testing" method :). But this is fine for now.

@jtcohen6
Copy link
Contributor Author

fire_event_if_testing

That's a reasonable suggestion! I know we previously had "test-only" events; this is basically the same concept.

@jtcohen6 jtcohen6 force-pushed the jerco/6671-rm-parse-events branch from bc8ffa2 to 9b091b6 Compare April 27, 2023 11:19
@jtcohen6 jtcohen6 merged commit e3498bd into main Apr 27, 2023
@jtcohen6 jtcohen6 deleted the jerco/6671-rm-parse-events branch April 27, 2023 16:17
@aranke aranke mentioned this pull request May 9, 2023
6 tasks
@github-actions
Copy link
Contributor

The backport to 1.5.latest failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.5.latest 1.5.latest
# Navigate to the new working tree
cd .worktrees/backport-1.5.latest
# Create a new branch
git switch --create backport-7388-to-1.5.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e3498bdaa5cdfeb6a52b2d1959ddbd4260fc7b8d
# Push it to GitHub
git push --set-upstream origin backport-7388-to-1.5.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.5.latest

Then, create a pull request where the base branch is 1.5.latest and the compare/head branch is backport-7388-to-1.5.latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CT-1864] Combine (Eliminate?) Parse Events

3 participants