Skip to content

[CLEANUP] Clean up ParserState a bit #1173

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
Merged
merged 1 commit into from
Mar 16, 2025
Merged

[CLEANUP] Clean up ParserState a bit #1173

merged 1 commit into from
Mar 16, 2025

Conversation

oliverklee
Copy link
Collaborator

No description provided.

@oliverklee oliverklee requested a review from JakeQZ March 16, 2025 12:26
@oliverklee oliverklee self-assigned this Mar 16, 2025
@oliverklee oliverklee mentioned this pull request Mar 16, 2025
Merged
@coveralls
Copy link

coveralls commented Mar 16, 2025 •
edited
Loading

Coverage Status

coverage: 55.906% (+0.2%) from 55.727%
when pulling e04d9d4 on cleanup/annotations/parserstate
into e0e0dd3 on main.

@oliverklee oliverklee force-pushed the cleanup/annotations/parserstate branch 2 times, most recently from dd594f0 to 5aa7231 Compare March 16, 2025 12:51
@oliverklee oliverklee force-pushed the cleanup/annotations/parserstate branch from 5aa7231 to e04d9d4 Compare March 16, 2025 16:26
JakeQZ
JakeQZ reviewed Mar 16, 2025
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

LGTM, though I wonder if the return type for consumeComment should also be changed as part of this PR - ?

src/Parsing/ParserState.php
return $result;
}

/**
* @return ($string is non-empty-string ? non-empty-string : string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't know this was possible.

@oliverklee
Copy link
Collaborator Author

LGTM, though I wonder if the return type for consumeComment should also be changed as part of this PR - ?

Are you thinking of improving the type annotation, or are you thinking it of changing it from Comment|false to ?Comment?

@JakeQZ JakeQZ mentioned this pull request Mar 16, 2025
Merged
@oliverklee
Copy link
Collaborator Author

Anyway, I have just created #1176 to use null as the "nothing" type instead.

@JakeQZ
Copy link
Collaborator

JakeQZ commented Mar 16, 2025

Are you thinking of improving the type annotation, or are you thinking it of changing it from Comment|false to ?Comment?

I added a review comment to the code, but it's disappeared (perhaps I forgot to click the 'Add review comment' button).

I was thinking it could be change to ?Comment, with null returned instead of false if there isn't a comment to consume.

JakeQZ
JakeQZ approved these changes Mar 16, 2025
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Since #1176 has been added to capture the outstanding issue, this can be merged.

@JakeQZ JakeQZ merged commit b3c1936 into main Mar 16, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/annotations/parserstate branch March 16, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeQZ JakeQZ JakeQZ approved these changes

Assignees

@oliverklee oliverklee

Labels
cleanup
Projects
None yet
Milestone
9.0.0: Drop support for PHP < 7.2 and...
Development

Successfully merging this pull request may close these issues.
3 participants
@oliverklee @coveralls @JakeQZ