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

DTZ005 false postitive for tz=None #10251

Closed
adriangb opened this issue Mar 6, 2024 · 6 comments · Fixed by #10621
Closed

DTZ005 false postitive for tz=None #10251

adriangb opened this issue Mar 6, 2024 · 6 comments · Fixed by #10621
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers rule Implementing or modifying a lint rule

Comments

@adriangb
Copy link
Contributor

adriangb commented Mar 6, 2024

import datetime

datetime.datetime.now(tz=None) 
# The use of `datetime.datetime.now()` without `tz` argument is not allowed Ruff DTZ005

ruff 0.1.9

@charliermarsh
Copy link
Member

I think this is arguably a true positive, since tz=None is equivalent to omitting it, in that it won't produce a timezone-aware object. Perhaps the documentation and message need to be tweaked to reflect that?

@charliermarsh charliermarsh added the documentation Improvements or additions to documentation label Mar 6, 2024
@adriangb
Copy link
Contributor Author

adriangb commented Mar 6, 2024

I think it’s arguable. No arguments could be a mistake (and is a common one). Explicitly passing None is acknowledging that you actually want None. In my case I want to use UTC everywhere except in a helper script.

But yes at least an error message tweak would be nice.

@charliermarsh
Copy link
Member

Yeah I also view it as borderline. I could definitely be convinced to ignore it. May need more opinions. @AlexWaygood, what do you think?

@charliermarsh charliermarsh added needs-decision Awaiting a decision from a maintainer and removed documentation Improvements or additions to documentation labels Mar 6, 2024
@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 7, 2024

It seems like we're following what the original flake8 rule does here, which is explicitly going out of its way to make sure that calls are still flagged when None is passed to this argument: https://github.com/pjknkda/flake8-datetimez/blob/1bcda18963ef61861a57c86f83741172884aadd1/flake8_datetimez.py#L96-L110. (But that's obviously not decisive -- if this is bad behaviour, we shouldn't do it!)

I... am sort-of inclined to leave this rule as it is, though. It's simple to explain currently: "aware datetimes are good; naive datetimes are bad". Keeping a rule simple to explain to users and easy to describe in documentation is valuable. Moreover, if we changed the behaviour here, it's possible people who don't understand the rationale behind the rule might think that they're "fixing" the issue by passing None (since the linter stops complaining at them). That wouldn't necessarily be true.

I feel like if you're working in a domain where you don't need aware timezones, you can probably just not enable this rule on your project. And if you have a specific script in a larger project where it's just that script that doesn't need aware timezones, you can probably just put # ruff: noqa: DTZ005 somewhere near the top of the file.

@adriangb
Copy link
Contributor Author

adriangb commented Mar 7, 2024

it's possible people who don't understand the rationale behind the ruule might think that they're "fixing" the issue by passing None (since linter stops complaining at them)

That's a fair point, I hadn't thought of that.

I guess the other recommendation is to do:

datetime.now(tz=timezone.utc).astimezone()

Which I think gives you the local timezone, although it's not as simple as tz=None

Either way, whatever you choose is fine by my, I just wanted to bring it to attention.

@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

I think we should just improve the rule documentation and recommendation here

@zanieb zanieb added documentation Improvements or additions to documentation good first issue Good for newcomers rule Implementing or modifying a lint rule and removed needs-decision Awaiting a decision from a maintainer labels Mar 11, 2024
cclauss added a commit to cclauss/ruff that referenced this issue Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers rule Implementing or modifying a lint rule
Projects
None yet
4 participants