Assert at least one tz arg is always UTC
State: closed

closes #17734

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Bug
name: Timezones
description: Timezone data dtype

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18228?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@40fd6b4`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18228/graphs/tree.svg?height=150&width=650&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/18228?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #18228   +/-   ##
=========================================
  Coverage          ?   91.39%           
=========================================
  Files             ?      163           
  Lines             ?    50091           
  Branches          ?        0           
=========================================
  Hits              ?    45779           
  Misses            ?     4312           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.2% <ø> (?)` | |
| #single | `40.36% <ø> (?)` | |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18228?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18228?src=pr&el=footer). Last update [40fd6b4...754dd7b](https://codecov.io/gh/pandas-dev/pandas/pull/18228?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> remove the first clause else (which just sets utc_date = val, then this is no longer necessary.

I think there may be some confusion about the underlying issue.  In the case where `utc_date = val`, you're right that the change in line 448/451 is irrelevant.  The problem is that in the case where `utc_date != val`, the change here may matter.

The new assertion here is a band-aid that ensures we never get to cases where the change here matters.  Ideally we shouldn't need that assertion.

The status-quo use of `val` in line 448 is an outlier that does not fit with either a) the rest of this function or b) the implementation in `tz_convert`.  The latter is the bug in question.  Changing `val` --> `utc_date` in line 451 is very much a good thing.

ok ping on green.

note you may wish to switch / use rebasing rather than merging as its much cleaner.
> ok ping on green.

The PR in its current state is a win because it avoids incorrect behavior, but if we can confirm that the usage in tz_convert (which is here the _new_ usage in tz_convert_single) is correct, then we can get rid of the assertion.

On the flip side, if we _cant_ confirm that the usage in tz_convert is correct, then we have an altogether different problem.

> note you may wish to switch / use rebasing rather than merging as its much cleaner.

This distinction has come up over in statsmodels.  I've just been running "git pull upstream master" whenever a rebase is needed.  I'll try to figure out the grown-up way of doing things.
> This distinction has come up over in statsmodels. I've just been running "git pull upstream master" whenever a rebase is needed. I'll try to figure out the grown-up way of doing things.

when we merge it doesn't matter as things get squashed to a single commit.

its cleaner for reviewing; every little commit doesn't matter. Logical commits matter.
thanks @jbrockmendel