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

Refactor: Replace try/except with contextlib.suppress() #8676

Merged
merged 1 commit into from Oct 5, 2022

Conversation

ahzamm
Copy link
Contributor

@ahzamm ahzamm commented Sep 28, 2022

If the desire is to simply suppress an error, rather than perform some sort of branching logic, the Python standard library has a paradigm for that: contextlib.suppress().

  • It reduces the code size and increases the readability of the code.
  • It is more elegant in terms of the coding style.

@tomchristie
Copy link
Member

Okay, this all looks neatly done. Thank you.

Unclear question to me - is it worth us continuing to accept refactorings on REST framework?

  • Pro: They might make the codebase arguably neater. Although not entirely clear, could be argued either way.
  • Con: Introduces code churn, risk, and busywork.

Do any folks who've been involved in maintaining REST framework have opinions on this? I'm probably okay either ways.

@kevin-brown
Copy link
Member

kevin-brown commented Oct 4, 2022

I can't figure out if this has a breaking change in behaviour because of how the if + 2 try...except was merged down to 1 suppression, specifically for ISO 8601 parsing returning None and continuing through before.

Do any folks who've been involved in maintaining REST framework have opinions on this?

Generally I'm -0 on things like refactoring for the sake of refactoring. With that being said, for this specific case I'm a +0/+1 because I'm very opposed to try...except...pass blocks, and much prefer the style and clarity of contextlib.suppress(). Of course, it's always a trade off in terms of time spent reviewing because as a maintainer we like to try to reduce risk with each pull request, and often refactoring PRs don't do that.

@tomchristie
Copy link
Member

Okay, yup - on balance I think we can go with this.

Thanks. 🙏🏼

@tomchristie tomchristie merged commit c10f226 into encode:master Oct 5, 2022
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.

None yet

4 participants