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

Drop casting which is effectively a no-op #6101

Merged
merged 1 commit into from
Apr 16, 2022

Conversation

jakirkham
Copy link
Member

In both of these cases mv is simply being cast to a format (and shape) that it already is. Though this was an accidental overwrite by a previous bug fix PR. The code previously tried to maintain mv's format and shape before performing a copy. While that behavior could be restored, the current behavior has been unchanged for over a year. This indicates the casting was likely unnecessary to begin with. So just drop it. If we find we need it after all, it is easy to add correctly, which require different code than what is here anyways.

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

In both of these cases `mv` is simply being cast to a `format` (and
`shape`) that it already is. Though this was an accidental overwrite by
a previous bug fix PR. The code previously tried to maintain `mv`'s
`format` and `shape` before performing a copy. While that behavior could
be restored, the current behavior has been unchanged for over a year.
This indicates the `cast`ing was likely unnecessary to begin with. So
just drop it. If we find we need it after all, it is easy to add
correctly, which require different code than what is here anyways.
@github-actions
Copy link
Contributor

Unit Test Results

       16 files  ±0         16 suites  ±0   7h 22m 29s ⏱️ - 15m 52s
  2 730 tests ±0    2 649 ✔️  - 1       80 💤 ±0  1 +1 
21 725 runs  ±0  20 647 ✔️  - 3  1 077 💤 +2  1 +1 

For more details on these failures, see this check.

Results for commit 99ec3d6. ± Comparison against base commit f6b2e03.

@jakirkham
Copy link
Member Author

Thoughts on this @dask/maintenance? 🙂

@martindurant
Copy link
Member

@jakirkham , I started #6104 . There are plenty of typos to grab yet, but I suppose this is the way forward.

@martindurant
Copy link
Member

@jakirkham , so sorry, this is in the wrong place. Never mind, now you know! :)

@jakirkham
Copy link
Member Author

Going to go ahead and merge since this code isn't doing anything anyways. Can always follow up if I've missed something though

@jakirkham jakirkham merged commit bc8efdf into dask:main Apr 16, 2022
@jakirkham jakirkham deleted the drop_unused_cast branch April 16, 2022 01:46
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.

2 participants