-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix deprecation warnings #1013
Fix deprecation warnings #1013
Conversation
✅ Deploy Preview for conda-store canceled.
|
✅ Deploy Preview for conda-store ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
7f45574
to
8705042
Compare
94d9445
to
85f1200
Compare
I did not go through all the changes in detail but this is much appreciated.
Can you add a TODO note on this in the code so we do not forget to do this when needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below about python 3.10/utcnow issue. I vote for a github issue and the suggested snippet instead of TODOs everywhere, but I'd be okay if you wanted to merge this as is too.
@@ -173,6 +173,7 @@ def __init__(self, specification, is_lockfile: bool = False): | |||
name: Mapped[str] = mapped_column(Unicode(255), nullable=False) | |||
spec: Mapped[dict] = mapped_column(JSON, nullable=False) | |||
sha256: Mapped[str] = mapped_column(Unicode(255), unique=True, nullable=False) | |||
# TODO: change to datetime.datetime.now(datetime.UTC) when python 3.10 is dropped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make an issue for this instead of inline TODOs? I know adding them was suggested before but they have a habit of getting lost in the repo if there aren't issues for them...
To mitigate datetime deprecation warnings, we could also do something like this at the top of the file (or in a utils.py
) so that the correct function is called on whatever version is currently running:
import sys
if sys.version_info[:2] > (3, 10):
from datetime.datetime import now
else:
from datetime.datetime import utcnow as now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo, I like the suggestion of making this an issue opposed to TODO's.
For the deprecation warnings, I don't think this snippet is going to quite work. datetime.datetime.now
will use the default TZ provided by the machine it's running on if it's not given the timezone parameter. And utcnow
does not accept any arguments. So on my machine (I'm on Pacific time) I get
>>> datetime.datetime.now()
datetime.datetime(2024, 12, 16, 12, 10, 50, 437991)
>>> datetime.datetime.utcnow()
. . . deprecation warning removed for brevity . . .
datetime.datetime(2024, 12, 16, 20, 11, 2, 237286)
>>> datetime.datetime.utcnow(datetime.UTC)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: datetime.utcnow() takes no arguments (1 given)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to issue: #1025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I guess it would have to be something like
import sys
if sys.version_info[:2] > (3, 10):
from datetime.datetime import now
else:
import datetime
def now():
return datetime.datetime.utcnow(datetime.UTC)
But I'm fine with just merging as is if that's preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'm going to go for a merge so we have less work in the future when we can drop python 3.10
4cc0428
to
cad48b9
Compare
Description
There are currently lots of deprecation warning when running the test suite. Some of them come from upstream dependencies. But many are from conda-store. This PR aims to resolve some of them.
not fixed:
datetime.datetime.utcnow
is deprecated, should be replaced withdatetime.datetime.now(datetime.UTC)
. However, this is not supported by python 3.10. Will address this when we drop python 3.10 supportPull request checklist