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

Tests for annotated string/char c-tors #56513

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Tests for annotated string/char c-tors #56513

wants to merge 3 commits into from

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Nov 9, 2024

These don't appear to be tested on Codecov.

@kshyatt kshyatt added test This change adds or pertains to unit tests strings "Strings!" labels Nov 9, 2024
@kshyatt kshyatt requested a review from tecosaur November 9, 2024 20:15
@kshyatt
Copy link
Contributor Author

kshyatt commented Nov 10, 2024

Sorry, wasn't able to test this locally (make test-strings refused to build a sysimage on my Fedora laptop), I'll try to resolve the test issues.

@tecosaur
Copy link
Contributor

Hi Katherine, thanks for your continued efforts to shore up test coverage!

Would you be able to point me to the particular source that led to these tests? At a glance they don't seem valid to me 😕

The Base.AnnotatedString("hello", :label => 1) syntax is the original form, but after it was raised that a pair-in-tuple structure was annoying to access elements of, I was asked to change it to a NamedTuple form, and so no longer expect this syntax to work: #55741 (as an aside: I still need to finish backporting these changes to the compat release (as time permits))

I think the first AnnotatedChar expression has this issue too, but the second Base.AnnotatedChar('h', [(1:1, :label, 1)]) includes a region, which doesn't simply apply to AnnotatedChars (they only hold a list of label-value associations).

@kshyatt
Copy link
Contributor Author

kshyatt commented Nov 10, 2024

@tecosaur
Copy link
Contributor

tecosaur commented Nov 13, 2024

Ah, I see. Using plain old tuples (instead of the expected named tuples) is probably what you want then, e.g.

a = AnnotatedString("hey", [(1:3, :attr, '7')])
@test a[1] == AnnotatedChar('h', [(:attr, '7')])

@kshyatt
Copy link
Contributor Author

kshyatt commented Nov 17, 2024

Looks like the test works, but uploading the coverage report failed

@inkydragon inkydragon added the merge me PR is reviewed. Merge when all tests are passing label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me PR is reviewed. Merge when all tests are passing strings "Strings!" test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants