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

Get string/boolean/integer property fallback #750

Merged
merged 13 commits into from Apr 19, 2020
Merged

Conversation

ravage84
Copy link
Member

Type: feature
Issue: Resolves none
Breaking change: no

Refers to #747 (comment)

I did not use the new fall back values it in any call ,yet.

Self critique::
Theoretically, this could lead to two cases of unclean code:

  1. It copies or changes the default value set in the rule set XML.
  2. It allows to use properties that do not exist in the rule set XML.

@ravage84 ravage84 added this to the 2.8.3 milestone Apr 15, 2020
Copy link
Member

@kylekatarnls kylekatarnls left a comment

Choose a reason for hiding this comment

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

  • Test are fine for me.
  • I see some possible improvements in the code and PHPDoc (see comments)
  • The getBooleanProperty should not return null (it's unclear when expecting a boolean) and the doc should enforce all $default type to be aligned to the @return of the method

src/main/php/PHPMD/AbstractRule.php Outdated Show resolved Hide resolved
src/main/php/PHPMD/AbstractRule.php Outdated Show resolved Hide resolved
src/main/php/PHPMD/AbstractRule.php Outdated Show resolved Hide resolved
src/main/php/PHPMD/AbstractRule.php Outdated Show resolved Hide resolved
kylekatarnls
kylekatarnls previously approved these changes Apr 16, 2020
@MarkVaughn
Copy link
Collaborator

MarkVaughn commented Apr 16, 2020

Here's my suggestion for shared code

  
    /**
     * Returns the value of a configured property
     *
     * Throws an exception when no property with <b>$name</b> exists
     * and no default value to fallback was given.
     *
     * @param string $name The name of the property, e.g. "ignore-whitespace".
     * @param mixed $default An optional default value to fallback instead of throwing an exception.
     * @return mixed The value of a configured property.
     * @throws \OutOfBoundsException When no property for <b>$name</b> exists and
     * no default value to fallback was given.
     */
    protected function getProperty($name, $default = null)
    {
        if (isset($this->properties[$name])) {
            return $this->properties[$name];
        }
        if ($default !== null) {
            return $default;
        }

        throw new \OutOfBoundsException('Property "' . $name . '" does not exist.');
    }
    
    /**
     * Returns the value of a configured property as a boolean
     *
     * Throws an exception when no property with <b>$name</b> exists
     * and no default value to fallback was given.
     *
     * @param string $name The name of the property, e.g. "ignore-whitespace".
     * @param bool $default An optional default value to fallback instead of throwing an exception.
     * @return bool The value of a configured property as a boolean.
     * @throws \OutOfBoundsException When no property for <b>$name</b> exists and
     * no default value to fallback was given.
     */
    public function getBooleanProperty($name, $default = null)
    {
        return in_array($this->getProperty($name, $default), array('true', 'on', 1, true), false);
    }

    /**
     * Returns the value of a configured property as an integer
     *
     * Throws an exception when no property with <b>$name</b> exists
     * and no default value to fallback was given.
     *
     * @param string $name The name of the property, e.g. "minimum".
     * @param int $default An optional default value to fallback instead of throwing an exception.
     * @return int The value of a configured property as an integer.
     * @throws \OutOfBoundsException When no property for <b>$name</b> exists and
     * no default value to fallback was given.
     */
    public function getIntProperty($name, $default = null)
    {
        return (int) $this->getProperty($name, $default);
    }ƒ

    /**
     * Returns the raw string value of a configured property
     *
     * Throws an exception when no property with <b>$name</b> exists
     * and no default value to fallback was given.
     *
     * @param string $name The name of the property, e.g. "exceptions".
     * @param string|null $default An optional default value to fallback instead of throwing an exception.
     * @return string The raw string value of a configured property.
     * @throws \OutOfBoundsException When no property for <b>$name</b> exists and
     * no default value to fallback was given.
     */
    public function getStringProperty($name, $default = null)
    {
        return (string) $this->getProperty($name, $default);
    }

Also fallback has no space

@ravage84
Copy link
Member Author

Also fallback has no space

@MarkVaughn "fall back" in the sense of "to fall back [to]", not a fallback.

https://dictionary.cambridge.org/de/worterbuch/englisch/fall-back
https://dictionary.cambridge.org/de/worterbuch/englisch/fallback

@ravage84
Copy link
Member Author

Here's my suggestion for shared code

I'm fine with your suggestion, though I would prefer to leave the third in_array() parameter as in my PR.
And if you guys insist to change it, please do that in a separate commit so the visibility of the change is higher (e.g. if somebody tracks the change through the git history).

@MarkVaughn MarkVaughn closed this Apr 16, 2020
@MarkVaughn MarkVaughn reopened this Apr 16, 2020
@MarkVaughn
Copy link
Collaborator

sorry for the misclicks :/

@MarkVaughn
Copy link
Collaborator

Not sure what

[Composer\Downloader\TransportException]  
  Peer fingerprint did not match      

has to do with this code.

@ravage84
Copy link
Member Author

has to do with this code.

@MarkVaughn nothing. It's an issue of Travis. I'll restart the job.

@tvbeek tvbeek merged commit 036256d into master Apr 19, 2020
@tvbeek tvbeek deleted the getStringProperty-fallback branch April 19, 2020 20:38
@kylekatarnls kylekatarnls modified the milestones: 2.8.3, 2.9.0 May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants