Skip to content

Conversation

@JakeQZ
Copy link
Collaborator

@JakeQZ JakeQZ commented Dec 9, 2025

This now does the selector-parsing that parse() used to do itself.

Should help with #1324 and
#1398 (comment)

This now does the selector-parsing that `parse()` used to do itself.

Should help with #1324 and
#1398 (comment)
@JakeQZ JakeQZ requested a review from oliverklee December 9, 2025 03:27
@JakeQZ JakeQZ self-assigned this Dec 9, 2025
@JakeQZ JakeQZ added the cleanup label Dec 9, 2025
}

/**
* @param array<int, Comment> $comments
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't get Stan to be happy with list<Comment>...

Copy link
Collaborator

Choose a reason for hiding this comment

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

My best guess is that that's because ParserState::consumeUntil has $comments annotated as @param array<int, Comment> $comments as well, and it's passed by reference.

We probably can change both type annotations to list (in a separate PR).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. That is indeed the case.

We probably can change both type annotations to list (in a separate PR).

#1418.

@coveralls
Copy link

Coverage Status

coverage: 62.804% (+0.06%) from 62.745%
when pulling 4a5cf65 on cleanup/parse-selectors-method
into 400edb2 on main.

Copy link
Collaborator

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

🧹

@oliverklee oliverklee merged commit e6b19f2 into main Dec 9, 2025
23 checks passed
@oliverklee oliverklee deleted the cleanup/parse-selectors-method branch December 9, 2025 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants