Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Feature: Add ability to define declare statements #169

Merged
merged 7 commits into from
Oct 5, 2019
Merged

Feature: Add ability to define declare statements #169

merged 7 commits into from
Oct 5, 2019

Conversation

icanhazstring
Copy link
Contributor

@icanhazstring icanhazstring commented Jun 17, 2019

This will add the ability to configure declare() statements
at the top of generated files.

I took the implementation suggestion from @GaryJones in #102 and changes it a bit.
I changed the behavior to be only able to generate valid declare statements at all.
Also I added static factories to generate the possible directives.

Should solve #102


One problem tho. Since declare is a reserved keyword. The I decided to go with Declare_ as the class representation. But the codesniffer doesn't like this. Any suggestions? Renamed to DeclareStatement as suggested by @webimpress

This adds the ability to configure declare() statements
at the top of generated files. It is only possible to
generate valid declare statements as stated in php documentation
https://www.php.net/manual/en/control-structures.declare.php

Solves #102
@michalbundyra
Copy link
Member

@icanhazstring I would go with DeclareStatement

@michalbundyra
Copy link
Member

@icanhazstring maybe I missed something but with that implementation is not possible to add declare like:

declare(strict_types=1,ticks=1);

but it's valid. It can have even all three declarations - strict_types, ticks and encodning.

@michalbundyra
Copy link
Member

@icanhazstring
Ok, nvm. I see, you are doing it on separate statements. It's fine. Maybe even better :)

@icanhazstring
Copy link
Contributor Author

icanhazstring commented Jun 17, 2019

@webimpress wanted to write this. Yes ;)
The other way around would be also possible, but I guess this is a matter of taste. I would have to change the FileGenerator to only have a single DeclareStatement. Maybe then we could remove the logic inside setDeclares to check if the declare is already given. Maybe some better SoC? Don't know ;)

src/DeclareStatement.php Outdated Show resolved Hide resolved
src/DeclareStatement.php Outdated Show resolved Hide resolved
src/DeclareStatement.php Outdated Show resolved Hide resolved
src/DeclareStatement.php Outdated Show resolved Hide resolved
src/Generator/FileGenerator.php Outdated Show resolved Hide resolved
src/Generator/FileGenerator.php Outdated Show resolved Hide resolved
test/Generator/FileGeneratorTest.php Outdated Show resolved Hide resolved
src/DeclareStatement.php Outdated Show resolved Hide resolved
src/DeclareStatement.php Outdated Show resolved Hide resolved
@michalbundyra michalbundyra added this to the 3.4.0 milestone Aug 28, 2019
@michalbundyra michalbundyra requested a review from Ocramius October 5, 2019 09:11
@michalbundyra michalbundyra changed the base branch from master to develop October 5, 2019 22:37
michalbundyra added a commit that referenced this pull request Oct 5, 2019
Feature: Add ability to define declare statements
michalbundyra added a commit that referenced this pull request Oct 5, 2019
michalbundyra added a commit that referenced this pull request Oct 5, 2019
@michalbundyra michalbundyra merged commit 6918dee into zendframework:develop Oct 5, 2019
@michalbundyra
Copy link
Member

Thanks, @icanhazstring!

@icanhazstring icanhazstring deleted the feature/declare branch October 10, 2019 11:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants