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

feat: rename toXXX methods #1919

Merged
merged 2 commits into from
Nov 29, 2022
Merged

feat: rename toXXX methods #1919

merged 2 commits into from
Nov 29, 2022

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Nov 28, 2022

This PR tackles a small naming inconsistency. Because these are public functions, we should decide now whether to merge this!


📚 The documentation for this PR will be available at https://awkward-array.readthedocs.io/en/agoose77-refactor-rename-to-xxx/ once Read the Docs has finished building 🔨

@agoose77
Copy link
Collaborator Author

@jpivarski I feel like we touched on this before but I don't recall the solution and can't find the discussion :/ If you're against this, close it, if in favour, merge! :)

@agoose77 agoose77 force-pushed the agoose77/refactor-rename-to-xxx branch from ecaa9cb to f838754 Compare November 28, 2022 23:09
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember it, too. I didn't like your earlier suggestions that made the whole method names lowercase (they should reflect the names of the classes they instantiate), but I was in favor of the underscore after "to," which this PR applies.

You're right that this is our last chance to fix it because it's public API. In fact, it's public API that Uproot uses, so we'll also need to coordinate releases. It's not used by dask-awkward, awkward-pandas, or Vector, which would be harder to coordinate with.

  1. I'll make a PR on Uproot that uses the new to_ names, but I will not merge it. You'll be the reviewer of that PR.
  2. We'll make another pre-release of Awkward this week. (Probably more than one.)
  3. Then when Uproot breaks, I'll merge that Uproot PR to unbreak it.
  4. There should probably be another pre-release of Uproot, to give people something to test.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #1919 (f838754) into main (88f7fef) will not change coverage.
The diff coverage is 83.21%.

❗ Current head f838754 differs from pull request most recent head 0968729. Consider uploading reports for the commit 0968729 to get more accurate results

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_broadcasting.py 93.38% <ø> (ø)
src/awkward/_connect/cling.py 25.40% <0.00%> (ø)
src/awkward/contents/indexedarray.py 76.60% <25.00%> (ø)
src/awkward/_util.py 82.46% <50.00%> (ø)
src/awkward/contents/bitmaskedarray.py 65.00% <59.25%> (ø)
src/awkward/contents/unmaskedarray.py 66.09% <80.00%> (ø)
src/awkward/contents/bytemaskedarray.py 87.88% <84.61%> (ø)
src/awkward/_slicing.py 86.04% <85.71%> (ø)
src/awkward/contents/numpyarray.py 89.70% <88.23%> (ø)
src/awkward/contents/regulararray.py 87.64% <92.30%> (ø)
... and 16 more

@agoose77
Copy link
Collaborator Author

I ran another test with rg "to[A-Z]" src and found a missing case.

@agoose77 agoose77 enabled auto-merge (squash) November 28, 2022 23:20
jpivarski added a commit to scikit-hep/uproot5 that referenced this pull request Nov 28, 2022
jpivarski added a commit to scikit-hep/uproot5 that referenced this pull request Nov 28, 2022
@agoose77 agoose77 merged commit 791d198 into main Nov 29, 2022
@agoose77 agoose77 deleted the agoose77/refactor-rename-to-xxx branch November 29, 2022 00:05
jpivarski added a commit to scikit-hep/uproot5 that referenced this pull request Dec 6, 2022
jpivarski added a commit to scikit-hep/uproot5 that referenced this pull request Dec 6, 2022
jpivarski added a commit to scikit-hep/uproot5 that referenced this pull request Dec 6, 2022
* refactor: adjust for name change in scikit-hep/awkward#1919.

* If awkward>=2.0.0rc5 contains scikit-hep/awkward#1919, it will become the new minimum version.
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