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.

Sign up for GitHub

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

Jump to bottom

Rename exceptions #6539

Merged
merged 34 commits into from
Jan 10, 2023
Merged

Rename exceptions #6539

merged 34 commits into from
Jan 10, 2023

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Jan 6, 2023 •
edited
Loading

resolves #6460

Description

Standardized exception classes to end in Error to meet PEP recommendations here.

Checklist

@cla-bot cla-bot bot added the cla:yes label Jan 6, 2023
@emmyoop emmyoop force-pushed the er/ct-1687-rename-exceptions branch 2 times, most recently from 85fd69b to 3cb7cbe Compare January 10, 2023 13:40
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@VersusFacit VersusFacit mentioned this pull request Jan 10, 2023
Merged
6 tasks
@emmyoop emmyoop marked this pull request as ready for review January 10, 2023 20:42
@emmyoop emmyoop requested a review from a team as a code owner January 10, 2023 20:42
@emmyoop emmyoop requested a review from a team January 10, 2023 20:42
@emmyoop emmyoop requested review from a team as code owners January 10, 2023 20:42
@emmyoop emmyoop requested review from nathaniel-may, ChenyuLInx, gshank and peterallenwebb and removed request for a team, nathaniel-may and ChenyuLInx January 10, 2023 20:42
@colin-rogers-dbt
Copy link
Contributor

do people rely on these exceptions? Is it worth making this change just to comply with pep style guide?

gshank
gshank approved these changes Jan 10, 2023
View reviewed changes
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.

Wow. So many lines changed... But consistency is a good thing.

colin-rogers-dbt
colin-rogers-dbt requested changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt left a comment

Choose a reason for hiding this comment

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

Do we have a sense on the broader impact this will have? Could this break services invoking us programmatically?

@emmyoop
Copy link
Member Author

emmyoop commented Jan 10, 2023 •
edited
Loading

@colin-rogers-dbt the old exceptions have been deprecated, not yet removed. So this should not break anyone immediately. We also do not currently guarantee this as an interface. And the conversations I've had internally seem to point to only using RPC exceptions which I did not touch.

Only 22 of the renamed exception existed before 1.4 and those are the ones deprecated. The rest are ones I created in a previous PR while deprecating our exception functions.

@emmyoop emmyoop force-pushed the er/ct-1687-rename-exceptions branch from b26b12b to ee154f0 Compare January 10, 2023 21:36
@emmyoop
Copy link
Member Author

emmyoop commented Jan 10, 2023

@colin-rogers-dbt and i had a conversation offline and decided to remove the deprecations on the classes. Leaving them deprecated has the potential to introduce bugs into adapters if they are catching specific exceptions. They will never catch the old exception, but because the class still exists they will also not break. While removing the old classes entirely will break anyone referring to the old exception classes, it will also prevent any silent bugs.

@emmyoop emmyoop merged commit eb200b4 into main Jan 10, 2023
@emmyoop emmyoop deleted the er/ct-1687-rename-exceptions branch January 10, 2023 23:43
@ueshin ueshin mentioned this pull request Jan 12, 2023
Merged
ueshin added a commit to databricks/dbt-databricks that referenced this pull request Jan 12, 2023
@ueshin
Follow up on renaming exceptions. (#251)
7ca5bf9 
### Description

Follows up on renaming exceptions.

- dbt-labs/dbt-core#6539
- dbt-labs/dbt-spark#585
@jtcohen6 jtcohen6 mentioned this pull request Jan 12, 2023
Merged
@greg-finley greg-finley mentioned this pull request Jan 25, 2023
Merged
1 task
@GabrielGLevine GabrielGLevine mentioned this pull request Jan 30, 2023
Closed
3 tasks
@iknox-fa iknox-fa mentioned this pull request Jan 30, 2023
Closed
6 tasks
@elyobo
Copy link

elyobo commented Jan 31, 2023

Some are certainly in use, e.g. see sqlfluff/sqlfluff#4325.

Removing deprecated stuff seems reasonable, but it does seem like something you'd want to do in a major release.

For others running into this, it seems like 1.3.x is the last before this breaking change.

bachng2017 added a commit to bachng2017/dbt-mindsdb that referenced this pull request Apr 9, 2023
@bachng2017
rename error class by dbt-labs/dbt-core#6539
206d7eb
@bachng2017 bachng2017 mentioned this pull request Apr 9, 2023
Merged
bachng2017 added a commit to bachng2017/dbt-mindsdb that referenced this pull request Apr 9, 2023
@bachng2017
rename error class by dbt-labs/dbt-core#6539
8a2bc2d
ZoranPandovski added a commit to mindsdb/dbt-mindsdb that referenced this pull request Apr 10, 2023
@ZoranPandovski
Merge pull request #23 from bachng2017/rename_error
207cb54 
rename error class by dbt-labs/dbt-core#6539
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-rogers-dbt colin-rogers-dbt colin-rogers-dbt approved these changes

@gshank gshank gshank approved these changes

@peterallenwebb peterallenwebb Awaiting requested review from peterallenwebb

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-1687] Rename every exception to end in Error
4 participants
@emmyoop @colin-rogers-dbt @elyobo @gshank