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

"Modernize" codebase #259

Merged
merged 13 commits into from Sep 28, 2020
Merged

"Modernize" codebase #259

merged 13 commits into from Sep 28, 2020

Conversation

Baptouuuu
Copy link
Contributor

@Baptouuuu Baptouuuu commented Sep 19, 2020

Need

As mentioned in #257 an upgrade of the codebase is interesting in order to type everything to help future upgrades.

Implementation

The following changes have been applied:

  • add the : void return type on all methods/functions that return nothing
  • add : int return type for Command::execute() as Symfony was moving to enforce developers to return an exit code
  • add all primitive types wherever possible (exceptions are for mixed or union types)
  • add vimeo/psalm as a dev requirement, configured on the strictest level
  • fixed all errors raised by psalm, mainly to provide type hints for command inputs and array templates
  • vendor/bin/psalm is now run in travis after phpunit
  • replace class strings with the FQCN::class pseudo constant, so tools and IDEs can better understand them
  • removed all docblocks that didn't provide information not already provided by the types
  • fixed an outdated php version in the preview command (as 7.1 il already enforced in composer.json

Comments

  • Since I added : void to the interface Step::__invoke it would require a new major release as it's a BC break. Depending on your release strategy this change may be rollbacked.
  • Since the PR changes quite a lot of code I suggest to review commit per commit
  • I suggest to replace, in another PR, Travis by GitHub actions. The benefits would be:
    • allow a better test matrix (os(linux, macOs), php(7.1, 7.2, 7.3, 7.4) and dependencies(lowest, normal))
    • split the website deploy job in it's own job (instead of an after success)
    • the split would allow to simplify the code of the travis-auto-deploy command

},
"config": {
"platform": {
"php": "7.1"
"php": "7.1.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to specify this bugfix version as it is the minimum required by vimeo/psalm

@@ -65,6 +66,10 @@ private function createTwig($templateDirectory)
]);

if (file_exists($templateDirectory.'/twig.php')) {
/**
* @psalm-suppress UnresolvableInclude
* @var callable(Twig_Environment): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we type with Twig_Environment but in the documentation of this feature you use Twig\Environment, should I replace Twig_Environment by the namespaced class name ? (There shouldn't be a difference since it's an alias, but I'm not sure of potential side effects)

Copy link
Member

Choose a reason for hiding this comment

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

Honestly go the way you prefer, I don't there should be side effects either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the composer.json refers twig ~1.10 the namespace isn't provided in this version, I'll change the documentation

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

That is awesome! I added a minor comment and then we're good to merge!

Thanks!

@@ -225,7 +225,7 @@ private function startLivereload(

private function isSupported(): bool
{
if (version_compare(phpversion(), '5.4.0', '<')) {
Copy link
Member

Choose a reason for hiding this comment

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

That isSupported() is for checking that PHP >= 5.4 because that's the version they added the built-in webserver.

We could completely remove that check now instead of upgrading the 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.

Done

@alanpoulain
Copy link
Contributor

Why not declaring strict typehint to all the files too?

@Baptouuuu
Copy link
Contributor Author

As suggested by @alanpoulain I added strict types everywhere

@mnapoli
Copy link
Member

mnapoli commented Sep 28, 2020

Awesome thanks!

@mnapoli mnapoli merged commit c168d0b into CouscousPHP:master Sep 28, 2020
@Baptouuuu Baptouuuu deleted the modernize-code branch September 28, 2020 09:28
@mnapoli mnapoli mentioned this pull request Sep 29, 2020
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.

None yet

3 participants