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

replace thecodingmachine/safe, webmozart/assert, and symfony/process by azjezz/psl #138

Merged
merged 2 commits into from
May 21, 2021

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented May 21, 2021

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

Similar to Roave/BackwardCompatibilityCheck#306, this PR replaces thecodingmachine/safe, webmozart/assert, and symfony/process by azjezz/psl.

This reduces the amount of dependencies, and relies on a better type assertion component provided by PSL.

Psl also ships with functions that replace PHP builtin to provide a better error handling, and consistency.

symfony/process is not made absolute by Psl\Shell\execute, as execute function is created as a replacement for exec builtin function, however, it provides enough functionality ( working directory, argument escaping, environment variables .. etc ) to replace the usage of symfony/process within this library ( if more features are needed later on, it makes sense to bring symfony/process back ).

@froschdesign
Copy link
Member

@azjezz

This reduces the amount of dependencies…

The number of dependent libraries is reduced, but the dependency to one library is increased. Because function calls that were previously based on classic PHP are replaced.
Therefore, I think that this argument does not work here.

(My statement refers only to this one argument and does not make any technical assessment!)

@azjezz
Copy link
Contributor Author

azjezz commented May 21, 2021

The number of dependent libraries is reduced, but the dependency to one library is increased

Yes, but that's -3 vs +1. so we are still down 2.

Because function calls that were previously based on classic PHP are replaced.

see Roave/BackwardCompatibilityCheck#306 (comment) and Roave/BackwardCompatibilityCheck#306 (comment) on why builtin functions were replaced.

@froschdesign
Copy link
Member

froschdesign commented May 21, 2021

@azjezz

…on why builtin functions were replaced.

Again: I have not made any technical assessment of the individual functions, I have only said that by replacing classical functions, the dependence to one library is increased. That is not a criticism or evaluation on Str\format or Str\trim_right itself!
And for that reason, the argument doesn't work for me – and I only mean this one argument!

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Stellar improvement! Very happy with the changes so far, so the comments are mostly about small details, and some type-related issues in the test suite.

psalm.xml.dist Outdated Show resolved Hide resolved
psalm.xml.dist Outdated Show resolved Hide resolved
src/Application/Command/CreateMilestones.php Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
src/Github/CreateReleaseTextViaKeepAChangelog.php Outdated Show resolved Hide resolved
.github/workflows/mutation-tests.yml Outdated Show resolved Hide resolved
@azjezz azjezz force-pushed the migration/psl branch 6 times, most recently from 6b861c8 to 89ecec8 Compare May 21, 2021 11:55
Signed-off-by: azjezz <azjezz@protonmail.com>
@Ocramius Ocramius added this to the 1.12.0 milestone May 21, 2021
Signed-off-by: azjezz <azjezz@protonmail.com>
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Excellent refactoring, thanks!

@Ocramius Ocramius self-assigned this May 21, 2021
@Ocramius Ocramius merged commit 04ee317 into laminas:1.12.x May 21, 2021
@azjezz azjezz deleted the migration/psl branch May 21, 2021 12:41
Ocramius added a commit that referenced this pull request Jun 4, 2021
These tests were not present, and would have regressed
in #13

Ref: #138 (comment)
Ref: #138 (comment)
Ref: #138 (comment)

Thanks @zerkms!

Assert::stringNotEmpty($value, sprintf('Could not find a value for environment variable "%s"', $key));
Psl\invariant($value !== null && $value !== '', Str\format('Could not find a value for environment variable "%s"', $key));
Copy link
Contributor

@bendavies bendavies Jun 7, 2021

Choose a reason for hiding this comment

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

@azjezz why not Type\non_empty_string()->matches($value) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we, you can do that too, same thing

Copy link
Contributor

@bendavies bendavies Jun 7, 2021

Choose a reason for hiding this comment

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

it doesn't work actually, I found.

ERROR: MoreSpecificReturnType - The declared return type 'non-empty-string' for EnvironmentVariables::getenv is more specific than the inferred return type 'string' (see https://psalm.dev/070)

ERROR: LessSpecificReturnStatement -- The type 'string' is more general than the declared return type 'non-empty-string' for EnvironmentVariables::getenv (see https://psalm.dev/129)

Copy link
Contributor

@bendavies bendavies Jun 7, 2021

Choose a reason for hiding this comment

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

ah, yes it does. i was trying

Psl\invariant(
    Type\non_empty_string()->assert($value),
    Str\format('Could not find a value for environment variable "%s"', $key)
);

instead of just

Type\non_empty_string()->assert($value);

but i guess you then lose the custom invariant error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, you lose the message with assert, maybe we could add a costume message as a second argument, not sure 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants