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

refactor!: make Content initialisers take nplike, parameters as keyword #1921

Merged
merged 16 commits into from
Nov 29, 2022

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Nov 29, 2022


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

@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #1921 (b3cd399) into main (4035aca) will decrease coverage by 0.00%.
The diff coverage is 91.46%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_broadcasting.py 93.38% <ø> (ø)
src/awkward/_connect/pyarrow.py 88.46% <ø> (ø)
src/awkward/_util.py 82.46% <ø> (ø)
src/awkward/contents/bitmaskedarray.py 65.00% <ø> (ø)
src/awkward/forms/bitmaskedform.py 86.07% <ø> (ø)
src/awkward/forms/bytemaskedform.py 86.30% <ø> (ø)
src/awkward/forms/indexedform.py 89.61% <ø> (ø)
src/awkward/forms/indexedoptionform.py 93.15% <ø> (ø)
src/awkward/forms/listform.py 82.71% <ø> (ø)
src/awkward/forms/recordform.py 88.57% <ø> (ø)
... and 33 more

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.

Yes, but also for Forms and Types, since there's a strong symmetry between these and Contents (especially Forms).

I'll get started on that, adding to this PR, if that's alright.

@agoose77
Copy link
Collaborator Author

agoose77 commented Nov 29, 2022

Fab, thanks for making this change. I was tempted to do them in multiple PRs, but on second thought it's less churn to do it all now. I didn't give it too much thought as I was playing around with semgrep to actually generate this PR

You can see the config that I used to do these renames, run with

semgrep -a --config=./rename.yaml
rename.yaml
rules:
- id: single-arg
  patterns:
    - pattern: '$F($X, $Y, $Z, $Q, $W, $U)'
    - metavariable-regex:
        metavariable: $F
        regex: ^.*ListArray$
  languages: [python]
  severity: WARNING
  message: Semgrep found a match
  fix: $F($X, $Y, $Z, $Q, $W, parameters=$U)

It wasn't perfect; sometimes it mangled the source. But careful staging worked well.

@agoose77 agoose77 enabled auto-merge (squash) November 29, 2022 19:43
@jpivarski
Copy link
Member

We were thinking along the same lines: I wanted to make them separate commits, rather than separate PRs, so that they can be checked in stages. It's just a question of what granularity the main branch should have. (After this refactoring is done, it's unlikely someone will want to see them separately, and if they do, they can follow the merge commit to this PR and see the individual commits.)

@agoose77 agoose77 enabled auto-merge (squash) November 29, 2022 19:50
@agoose77
Copy link
Collaborator Author

I ran a check over the forms with

rules:
- id: single-arg
  patterns:
    - pattern: '$F(...)'
    - pattern-not: '$F(..., parameters=$X)'
    - metavariable-regex:
        metavariable: $F
        regex: ^.*Type$
  languages: [python]
  severity: WARNING
  message: Semgrep found a match

and visually ensured the correct results

@agoose77 agoose77 enabled auto-merge (squash) November 29, 2022 19:54
@agoose77 agoose77 merged commit 3edac9b into main Nov 29, 2022
@agoose77 agoose77 deleted the agoose77/refactor-content-keyword branch November 29, 2022 19:55
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