Skip to content

Writing defaults of class properties #29

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
Jan 7, 2020
Merged

Writing defaults of class properties #29

merged 3 commits into from
Jan 7, 2020

Conversation

stephanniewerth
Copy link
Contributor

If you work with the rendered classes to export data, there are no default values for the class properties. With this pull request the default values will be written to the generated files. The setter methods will not get any defaults, because this should only be done in the definition of the class properties.

Class properties (first without a default value):

    /** @var Taxonomy TBD. */
    public $taxonomy;

    /** @var string */
    public $type = 'article';

    /** @var string The version of this object in major.minor.patch format. See also: https://semver.org/. */
    public $version = '0.0.0';

Setter would be the same as before:

public function setVersion($version)
{
    $this->version = $version;
    return $this;
}

There should not be any defaults in the setter methods of class properties. These should only be assigned in the initialization process of an object (constructor).
@@ -176,6 +176,10 @@ private function makeClass($schema, $path)
$phpProperty->addMeta($property, self::SCHEMA);
$phpProperty->addMeta($name, self::PROPERTY_NAME);

if (!is_null($property->default)) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this configurable, rendering default can be unwanted in some cases, e.g. to avoid default values when encoding json.

We can have some option like this in PhpBuilder:

    /**
     * Use default values to initialize properties
     * @var bool
     */
    public $declarePropertyDefaults = false;

and then

                if (!is_null($property->default) && $this->declarePropertyDefaults) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

@codecov
Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #29 into master will increase coverage by 0.78%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #29      +/-   ##
============================================
+ Coverage     88.23%   89.02%   +0.78%     
- Complexity      500      503       +3     
============================================
  Files            38       38              
  Lines          1326     1330       +4     
============================================
+ Hits           1170     1184      +14     
+ Misses          156      146      -10
Impacted Files Coverage Δ Complexity Δ
src/PhpFunction.php 91.07% <100%> (+0.16%) 24 <0> (+1) ⬆️
src/Property/Setter.php 100% <100%> (ø) 2 <0> (ø) ⬇️
src/JsonSchema/PhpBuilder.php 83.62% <100%> (+0.19%) 66 <0> (+2) ⬆️
src/PhpClassProperty.php 74.07% <0%> (+11.11%) 11% <0%> (ø) ⬇️
src/PhpNamedVar.php 78.26% <0%> (+15.21%) 18% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4535bc2...8794840. Read the comment docs.

@vearutop vearutop merged commit 0feaba7 into swaggest:master Jan 7, 2020
@stephanniewerth stephanniewerth deleted the feature/writing-properties-default-to-generated-class branch January 7, 2020 11:17
@vearutop
Copy link
Member

vearutop commented Jan 7, 2020

@derjumpy thank you, v0.2.24 tagged.

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.

2 participants