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

chore: cleanup flake8 #527

Merged
merged 10 commits into from
Jul 9, 2022
Merged

Conversation

henryiii
Copy link
Member

This cleans up some flake8 things.

setup.cfg Outdated Show resolved Hide resolved
@henryiii henryiii marked this pull request as draft December 10, 2021 20:10
@jpivarski
Copy link
Member

We're getting ready to split the Uproot repo into main, which will swap Awkward 1 for Awkward 2, and main-v4, which will be for bug-fixes to Uproot 4 (starting with 4.3.0). It's hard to maintain two branches, so I'd like to close old pull requests. Is there any part of this that you'd like to see merged into main and main-v4 or just main?

Note: this was never merged or reviewed because it was a draft. I assumed that you were still planning on adding to it, though it's been dormant for a while. If you want, we can apply it to main before the split, so that this, as it is, will apply to both Uproot 4 bug-fixes and Uproot 5.

@jpivarski
Copy link
Member

The failing test is the one with ROOT. One of the changes in main makes tests of ROOT behavior less strict, since the way that they handle TStreamerInfo when updating a file has apparently changed.

@henryiii
Copy link
Member Author

I marked it with draft because I need to solve the == None / is None problem (which is a PyROOT Python antipattern). Then I forgot about it. I can see if it can be revived.

@jpivarski
Copy link
Member

Since it's isolated, just a few places in the codebase and all of them in tests, I think the "# noqa: E711 (ROOT null check)" comments are acceptable. They're noisy and really call attention to the fact that this is not the normal way of doing things.

Even if ROOT changes, there are versions of ROOT out there that require == None. I suppose we could require a very recent version of ROOT for the tests, though I'd rather not if it's just for this. I'd be more inclined to find another way of writing the tests! (These tests require the non-existence of a LeafCount... Maybe there's a way to do it, other than the null-pointer check.)

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
Copy link
Member Author

Could it be assert not nb2.GetLeaf("nb2").GetLeafCount()? That would also trigger if GetLeafCount returned 0, is that a possibility to worry about?

Signed-off-by: Henry Schreiner <[email protected]>
@jpivarski
Copy link
Member

Could it be assert not nb2.GetLeaf("nb2").GetLeafCount()? That would also trigger if GetLeafCount returned 0, is that a possibility to worry about?

The return value of TLeaf::GetLeafCount is typed (TLeaf*), so only checking to see if it's "falsy" would be enough.

It's a nice way out. I tend to avoid relying on truthy and falsy tests, but my reasons are undermined by cases that have a checked type.

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii marked this pull request as ready for review June 21, 2022 18:59
@jpivarski jpivarski enabled auto-merge (squash) July 9, 2022 03:43
@jpivarski jpivarski merged commit 4f73917 into scikit-hep:main Jul 9, 2022
Moelf pushed a commit to Moelf/uproot5 that referenced this pull request Aug 1, 2022
* style: cleanup flake8

* style: more flake8

* style: clean a few more suggestions

* fix: check for print statements

* fix: restore a few more warnings

* fix: update a few versions

Signed-off-by: Henry Schreiner <[email protected]>

* chore: ignore prints in dev

Signed-off-by: Henry Schreiner <[email protected]>

* tests: noqa and use == on None

Signed-off-by: Henry Schreiner <[email protected]>

* tests: direct falsiness for ROOT

Signed-off-by: Henry Schreiner <[email protected]>

Co-authored-by: Jim Pivarski <[email protected]>
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