Deprecate PPP_CONFIG_DIR for specifying config path #170
Conversation
|
Oh I also made pytest more strict and removed usage of datetime utcnow. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
==========================================
+ Coverage 87.94% 87.97% +0.03%
==========================================
Files 16 16
Lines 2306 2312 +6
==========================================
+ Hits 2028 2034 +6
Misses 278 278
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
I don't see |
What do you mean? I am not following, sorry |
Above @djhoese said
I don't see anything related to |
|
Ah yep. I'm just waking up and won't be in front of the computer for a while (kid's doctor appointment). I'll see if I can push it while getting kids ready for the day. |
| @@ -617,6 +611,9 @@ def table_exists(db, name): | |||
| return db.execute(query, (name,)).fetchone() is not None # nosec | |||
|
|
|||
|
|
|||
| def _utcnow(): | |||
| return dt.datetime.now(tz=dt.timezone.utc).replace(tzinfo=None) | |||
There was a problem hiding this comment.
why do we want to remove the timezone info again?
There was a problem hiding this comment.
To make them naive. Keeping the timezone would require a lot more work I assume. For example, sanitizing user inputs.
There was a problem hiding this comment.
If the datetimes are timezone-unaware, they can't be compared with tz-aware datetimes. I thought our goal was to make everything tz-aware across the Pytroll ecosystem.
There was a problem hiding this comment.
And I think making all of the datetimes tzinfo=None is even more work than make them use UTC. The None case would mean addinitional TZ conversion for pretty much everywhere where we declare the times as UTC.
There was a problem hiding this comment.
I don't disagree that we should/could move toward UTC-aware times everywhere, but that wasn't my goal with my changes in this PR. I was mostly concerned about the warnings generated while running tests. I understand about 2% of pyorbital and the TLE portions, on a code level, are not part of that understanding. I should not be the person who does that conversion/translation in pyorbital. Maybe I'm overestimating how much datetimes are involved. If so I can take a stab at it, but I'm still not sure this is the PR for it.
|
oh, looks like tests are failing... |
|
Since this requires a 1.9 release I'll let others (@adybbroe ?) answer whether or not this should be merged and released in 1.9 next or if there should be a 1.8.x release first. |
| @@ -617,6 +621,9 @@ def table_exists(db, name): | |||
| return db.execute(query, (name,)).fetchone() is not None # nosec | |||
|
|
|||
|
|
|||
| def _utcnow(): | |||
| return dt.datetime.now(tz=dt.timezone.utc).replace(tzinfo=None) | |||
There was a problem hiding this comment.
I'll just add the official "request changes" for this bit with regards to tz aware/unaware I commented earlier.
This is a continuation of #169 to more fully deprecate PPP_CONFIG_DIR.
This may need updating to have it raise an exception completely. Further discussion is needed. Due to the previous deprecation warning, this PR should not be released until 1.9 is ready.
CC @pnuu @mraspaud @adybbroe
flake8 pyorbital