Skip to content

[TASK] Use native type declarations for $lineNumber #1134

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

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

oliverklee
Copy link
Collaborator

Part of #811

@coveralls
Copy link

coveralls commented Mar 10, 2025

Coverage Status

coverage: 55.567%. remained the same
when pulling a2a9b84 on task/types/line-number
into 6607f94 on main.

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.

There's a duplicated parameter name in one DocBlock.

I made some suggestions, perhaps for a separate PR, where I think $lineNumber is necessarily always at least 1. Though given that this PR already includes one change to int<1, max>, perhaps those should be included here. I'll leave it to your discretion...

@@ -20,9 +20,9 @@ class Parser

/**
* @param string $text the complete CSS as text (i.e., usually the contents of a CSS file)
* @param int<0, max> $lineNumber the line number (starting from 1, not from 0)
* @param int<1, max> $lineNumber the line number (starting from 1, not from 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sharp.

@@ -45,15 +45,15 @@ class ParserState
private $charset;

/**
* @var int
* @var int<0, max> $lineNumber
Copy link
Collaborator

@JakeQZ JakeQZ Mar 10, 2025

Choose a reason for hiding this comment

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

I wonder if $lineNumber can ever be 0 here. 0 is used in the various objects to indicate that no line number is available (e.g. for something added to the Document after parsing), but I think ParserState always has a valid line number which is greater than 0. Maybe beyond the scope of this PR.

@@ -45,15 +45,15 @@ class ParserState
private $charset;

/**
* @var int
* @var int<0, max> $lineNumber
*/
private $lineNumber;

/**
* @param string $text the complete CSS as text (i.e., usually the contents of a CSS file)
* @param int<0, max> $lineNumber
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto here. The default parameter value is 1, which sort-of implies it will always be at least 1.

@oliverklee oliverklee requested a review from JakeQZ March 11, 2025 08:13
@JakeQZ JakeQZ merged commit 5493982 into main Mar 11, 2025
21 checks passed
@JakeQZ JakeQZ deleted the task/types/line-number branch March 11, 2025 15:03
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.

3 participants