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.

Sign up for GitHub

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

Jump to bottom

fix: support const in typename #1231

Merged
merged 8 commits into from
Jan 17, 2025
Merged

fix: support const in typename #1231

merged 8 commits into from
Jan 17, 2025

Conversation

veprbl
Copy link
Contributor

@veprbl veprbl commented Jun 12, 2024

Resolves: #1229

@veprbl veprbl changed the title Support const in typename fix: Support const in typename Jun 12, 2024
@veprbl veprbl changed the title fix: Support const in typename fix: support const in typename Jun 12, 2024
@veprbl veprbl force-pushed the pr/support_const_in_typename branch from 6ddb559 to 72076e2 Compare June 12, 2024 23:45
@jpivarski jpivarski added the inactive A pull request that hasn't been touched in a long time label Nov 7, 2024
@veprbl veprbl force-pushed the pr/support_const_in_typename branch from 75d950b to 5836b12 Compare January 16, 2025 20:08
veprbl added 2 commits January 16, 2025 15:23
@veprbl
workaround test failure
b2928b3 
When full test suite is ran the following error is seen:
```
____________________________ test_const_in_typename ____________________________

    def test_const_in_typename():
>       assert parse_typename("TH1I*") == AsPointer(Model_TH1I)
E       AssertionError: assert AsPointer(Model_TH1I) == AsPointer(Model_TH1I)
E        +  where AsPointer(Model_TH1I) = parse_typename('TH1I*')
E        +  and   AsPointer(Model_TH1I) = AsPointer(Model_TH1I)

tests/test_1229_const_in_typename.py:12: AssertionError
```

Running just the `tests/test_1229_const_in_typename.py` doesn't show that.
Not clear what re-defines the types/modules.
@veprbl
add back the file-based test (since it was added to skhep_testdata)
e1357aa
@veprbl veprbl marked this pull request as ready for review January 16, 2025 20:29
@veprbl
Copy link
Contributor Author

veprbl commented Jan 17, 2025

This is ready for review. I could not resolve one of the issues with a test (see b2928b3), and instead found a reasonable alternative way to do the same test.

@jpivarski jpivarski removed the inactive A pull request that hasn't been touched in a long time label Jan 17, 2025
@jpivarski
Copy link
Member

Thanks for getting back to this! The strategy of checking for "const" at the very beginning and ignoring it thereafter sounds good to me, although when users ask for the typename, it will lack the const keywords, which they might be expecting. But I guess that's fine: the const keyword doesn't mean much for serialized data.

It should not be necessary to apply the workaround described in b2928b3. I checked out this branch and

>>> parse_typename("TH1I*") == AsPointer(Model_TH1I)
True
>>> parse_typename("const TH1I*") == AsPointer(Model_TH1I)
True

Moreover, this is not how the tests are failing—they're failing because they can't access test files through HTTP. It seems like some server is down. I'm running the tests again in main, and we'll see if there's an error there.

I should also look through the parse_typename for other places where we explicitly check for "const" because your change would have made them dead code (you can't get "const" later if you've already left at the beginning of the function). Pushing that modification would trigger the tests again. If it was a momentary HTTP server failure, they would pass.

@jpivarski
Copy link
Member

The tests in main are going to fail. It's because example.com has changed how it works, and we had been using it to test what (we thought) should be HTTP 206 errors, but it's now returning 200 (no error).

#1362 is an adjustment for that, and if it works, it will merge into main and then can be merged into this PR branch, which will fix the tests.

@jpivarski
Copy link
Member

In the two now-dead code blocks that I removed, I noticed that our old policy had always been to ignore the const keyword in type names, so that aspect is not changing.

jpivarski
jpivarski approved these changes Jan 17, 2025
View reviewed changes
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 wrote a lot of inline comments about the test failures and other updates. But when all test pass (by merging #1362 into main and main into this PR), I think this can be merged. Let me know, @ianna, if you disagree!

ianna
ianna approved these changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great. The tests should pass now.

@ianna ianna enabled auto-merge (squash) January 17, 2025 08:12
@ianna ianna merged commit c27d295 into scikit-hep:main Jan 17, 2025
26 checks passed
@veprbl veprbl deleted the pr/support_const_in_typename branch January 18, 2025 00:39
@veprbl
Copy link
Contributor Author

veprbl commented Jan 18, 2025

I appreciate the fixups to the existing code. My recollection was that const types would outright not parse without my change, so I did not think of looking for those cases.

@jpivarski
Copy link
Member

jpivarski commented Jan 18, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpivarski jpivarski jpivarski approved these changes

@ianna ianna ianna approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

parse_typename fails on const pointer members
3 participants
@veprbl @jpivarski @ianna