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

Logging README #4395

Merged
merged 5 commits into from
Dec 2, 2021
Merged

Logging README #4395

merged 5 commits into from
Dec 2, 2021

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Dec 2, 2021

Description

Update logging README to reflect current state

Checklist

  • 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
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Dec 2, 2021
@emmyoop emmyoop changed the title Er/logging readme Logging README Dec 2, 2021
@emmyoop emmyoop requested a review from nathaniel-may December 2, 2021 17:38
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Just a few minor bits. Otherwise this looks great to me


## Optional (based on your event)

- Events associated with node status changes must have `report_node_data` passed in and be extended with `Cache`
Copy link
Contributor

Choose a reason for hiding this comment

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

we should mention extending the NodeInfo class if that's right.

core/dbt/events/README.md Outdated Show resolved Hide resolved
## Required for Every Event

- A unique `code` that indicates the type
- a loglevel
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd follow suit with the 4th bullet and mention extending the class with an example.

core/dbt/events/README.md Outdated Show resolved Hide resolved
core/dbt/events/README.md Show resolved Hide resolved
@emmyoop emmyoop requested a review from nathaniel-may December 2, 2021 21:12
@emmyoop emmyoop marked this pull request as ready for review December 2, 2021 22:05
## Optional (based on your event)

- Events associated with node status changes must have `report_node_data` passed in and be extended with `Cache`
- define `asdict` if your data is not serializable
Copy link
Contributor

Choose a reason for hiding this comment

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

serializable to json

@emmyoop emmyoop requested a review from nathaniel-may December 2, 2021 22:30
@emmyoop emmyoop merged commit c675c2d into main Dec 2, 2021
@emmyoop emmyoop deleted the er/logging-readme branch December 2, 2021 23:04
leahwicz pushed a commit that referenced this pull request Dec 2, 2021
* WIP

* more README cleanup

* readme tweaks

* small tweaks

* wording updates
@leahwicz leahwicz mentioned this pull request Dec 2, 2021
4 tasks
leahwicz added a commit that referenced this pull request Dec 2, 2021
* WIP

* more README cleanup

* readme tweaks

* small tweaks

* wording updates

Co-authored-by: Emily Rockman <[email protected]>
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.

3 participants