Skip to content

use one line boolean condition with add types#9666

Closed
ArtemIsmagilov wants to merge 8 commits intoencode:masterfrom
ArtemIsmagilov:one-line-boolean
Closed

use one line boolean condition with add types#9666
ArtemIsmagilov wants to merge 8 commits intoencode:masterfrom
ArtemIsmagilov:one-line-boolean

Conversation

@ArtemIsmagilov
Copy link
Copy Markdown

simple refactoring

lovelydinosaur
lovelydinosaur previously approved these changes Mar 21, 2025
Copy link
Copy Markdown
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

The valid_datetime change is cleaner, okay. We're not generally accepting these types of PRs, tho one part of this is sufficiently obvious and reasonable. (We could put even harder boundaries in place here?)

Some suggested changes here to keep the codebase consistent.

Comment thread rest_framework/utils/timezone.py Outdated
Comment thread rest_framework/utils/timezone.py Outdated
Comment thread rest_framework/utils/timezone.py Outdated
Comment thread rest_framework/utils/timezone.py Outdated
Comment thread rest_framework/utils/timezone.py Outdated
Comment thread rest_framework/utils/timezone.py Outdated
@browniebroke
Copy link
Copy Markdown
Member

We're not generally accepting these types of PRs, tho one part of this is sufficiently obvious and reasonable. (We could put even harder boundaries in place here?)

I think we need to behave consistently in accepting these kind of PRs: we either accept them or we don't. Yes it's a trivial change but accepting this will IMO open the door to others (e.g. might be harder to decline these #9286 #9285).

I looked up to see if there was a clear technical benefit (from the machine perspective) of making this change and didn't find a compelling argument. Ruff has a page for each rule that usually make a good case for them and according to this one, it seems to be mostly about readability...

With that in mind, I'm -1 for this change, unless we find a way to rephrase the note from our contributing page, which currently reads:

At this point in its lifespan we consider Django REST framework to be feature-complete. We focus on pull requests that track the continued development of Django versions, and generally do not accept new features or code formatting changes.

@lovelydinosaur
Copy link
Copy Markdown
Contributor

With that in mind, I'm -1 for this change, unless we find a way to rephrase the note from our contributing page, which currently reads

🫡

@lovelydinosaur
Copy link
Copy Markdown
Contributor

Seems reasonable to me, @browniebroke.

Less noise less churn please.

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.

3 participants