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

[2.x/3.x] Add matchNotNull and other *NotNull implementations #14

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Nov 14, 2022

I'd be happy for a sanity check here before proceeding and adding the rest of them (matchWithOffsetsNotNull, matchAllNotNull, matchAllWithOffsetsNotNull, replaceCallbackNotNull, replaceCallbackArrayNotNull, isMatchNotNull, isMatchAllNotNull, isMatchWithOffsetsNotNull, isMatchAllWithOffsetsNotNull 😅 ).

The issue is that due to the improvements in PHPStan and https://github.com/composer/pcre/releases/tag/3.0.2 you now get errors everywhere because $match[1] for ex is string|null even tho in many cases the null will never happen. This is however dependent on the pattern and thus cannot really be asserted easily in the types.

So adding these NotNull variants for all functions outputting nullable matches solves this by letting you opt in to stricter non-nullable types, and if you messed up and your pattern does have null/unmatched-subpatterns you get an exception.

The main problems I see and where I'd love feedback are:

  • is the NotNull name clear/confusing?
  • is it polluting the interface too much? It adds lots of "duplicate" methods but they are technically different and kinda similar to the WithOffset ones we already have. This makes me think perhaps the name should be WithoutNull tho.

/cc @johnstevenson @naderman @stof

@Seldaek Seldaek added the enhancement New feature or request label Nov 14, 2022
@naderman
Copy link
Member

@glaubinix probably something you want to take a look at too.

src/Preg.php Show resolved Hide resolved
* @param 0|positive-int $count
* @param array<string> $matches
*/
public function __construct($count, array $matches)
Copy link

Choose a reason for hiding this comment

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

should have a int parameter type

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, looks like I forgot the result classes when working on the initial 2.0 changes. Will fix this.

@johnstevenson
Copy link
Member

is the NotNull name clear/confusing?

While it seems clear in isolation, in practice it starts getting a bit nightmarish (to me, anyway).

For example, when reading isMatchAllWithOffsetsNotNull and I start losing the will to live after WithOffsets. How about appending Strict to these function names instead.

Now isMatchAllWithOffsetsStrict immediately appears to my brain as the isMatchAllWithOffsets function with stricter requirements.

@Seldaek
Copy link
Member Author

Seldaek commented Nov 15, 2022

I start losing the will to live after WithOffsets

😆 hence why I asked.

Strict sounds ok too. But to be honest I think we also could omit a few, like the WithOffsets ones, because they are less commonly needed and so it's not that bad if you have to check for nulls sometimes. But the isMatch and other common ones trigger 150 errors in the Composer codebase alone..

@johnstevenson
Copy link
Member

I think we also could omit a few, like the WithOffsets ones

Yes, that seems reasonable. It would also make the NotNull suffixes not quite so mentally draining. However, I'm still not sure because isMatchNotNull and matchNotNull return truth(y) values and it doesn't seem immediately obvious that the NotNull is refering to a missing sub-pattern. Your WithoutNull suggestion is closer (but only if you know what is going on).

Perhaps isMatchStrictGroups would be better, or perhaps I'm over-thinking this!

@Seldaek
Copy link
Member Author

Seldaek commented Nov 15, 2022

Yeah I think I like StrictGroups, it's descriptive at least. For sure it'll confuse at first but that's probably the case no matter the name here given it's kind of a small distinction.

@Seldaek Seldaek merged commit 963cd62 into composer:main Nov 16, 2022
@Seldaek Seldaek deleted the not_null branch November 16, 2022 15:40
Seldaek added a commit that referenced this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants