-
Notifications
You must be signed in to change notification settings - Fork 89
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
refactor: reduce qualified names of Content, Form, Type subclasses #1757
refactor: reduce qualified names of Content, Form, Type subclasses #1757
Conversation
Codecov Report
Additional details and impacted files
|
This looks good to me. I've been earmarking this for a while.
Were you definitely getting v2 there? The fact that we have In any case, this PR looks good; I tested |
@jpivarski what do you want to do w.r.t the tests? I think we can also change those too. |
I don't understand the reason why it was not finding the name. I verified the versions. I got another error for not finding |
"Cleanup" type things are not very important in tests. We'll likely have to look at the main codebase frequently, making changes frequently, but a test should ideally just sit there, unchanged. Occasionally, they'll get changed, but it's a lot less than the main codebase. |
I'll be merging this as-is because we want to do the name change anyway. It got past some Uproot errors, but not all, and we shouldn't extend it to handle cases like
I don't understand (1) why it doesn't see the submodule |
I'll just mirror my comment here for posterity; The reason that our |
Specifically,
and
When pip-installed,
ak.types
doesn't seem to havelisttype
, etc., but it does haveListType
. Wait—could it be because MacOS is case-sensitive? No, if that's it, then this would have failed before.Regardless of whether this fixes the issue I see in using Uproot with Awkward's
main
, it's a name change we've been wanting to do.