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

[Tracking ticket] Fix coding style issues #899

Closed
JeroenDeDauw opened this Issue Mar 19, 2015 · 14 comments

Comments

7 participants
@JeroenDeDauw
Member

JeroenDeDauw commented Mar 19, 2015

We want our code to be compliant with our own coding style rules. Recently we created an automated way of detecting violations, making it trivial to get a list of violations. Most violations are very easy to fix.

To get a list of violations, first get a git clone of the repository. If you want to be able to contribute back, you probably first want to create a fork, and clone that.

git clone git@github.com:YourGitHubAccount/SemanticMediaWiki.git

Then change into the directory and run composer install

cd SemanticMediaWiki
composer install

If you do not have Composer installed yet, first install it.

Next install PHPCS in whatever manner suits you best. For instance like this:

curl -OL https://squizlabs.github.io/PHP_CodeSniffer/phpcs.phar

Once that is done, you can get a list of violations by running the following in the root SMW directory:

php phpcs.phar src/* includes/* tests/* --standard=phpcs.xml --extensions=php -sp

The output will look as follows (though will have the above command instead of compsoser cs-standalone)

Console output

Console output

You do not need to actually install SMW itself. So you do not need a working MediaWiki installation, a database and a webserver. Even if you have an actual SMW install, it might still be easier to work with a dedicated development version for this task.

Once you fixed a violation or a set of related violations, create a commit, push it to your fork, and create a pull request.

More advanced usage

If you create an actual working install of SMW, you can run composer cs, which will execute additional code style checks.

composer cs and composer cs-standalone only run style checks in the src directory. You can look at how those commands are defined and easily see how you can run them for other directories, such as includes and tests. Example:

vendor/bin/phpcs includes/* --standard=phpcs.xml --extensions=php -sp

The tools used are PHPCS and PHPMD, both of which allow you to run a single rule at a time, rather than the whole ruleset we have defined. This is useful if you only want to fix a single category of issues, such as spacing issues, or want to ignore noise from another category.

@JeroenDeDauw

This comment has been minimized.

Member

JeroenDeDauw commented Mar 19, 2015

@kghbln @mwjames I'm thinking of sending this to the list as another easy way to contribute. Do you think the instructions are clear enough? It does not explain how to install git, how to fork a repo and how to push things back to github, though perhaps the task is not suited for those that need to be brought up to speed with these steps?

@mwjames

This comment has been minimized.

Contributor

mwjames commented Mar 19, 2015

I'm thinking of sending this to the list as another easy way to contribute

I'd say go for it.

It does not explain how to install git, how to fork a repo and how to push things back to github, though perhaps the task is not suited for those that need to be brought up to speed with these steps?

Certainly true.

@kghbln

This comment has been minimized.

Member

kghbln commented Mar 19, 2015

+1 per @mwjames

Probably I could even do some of this. :)

ckoerner added a commit to ckoerner/SemanticMediaWiki that referenced this issue Mar 19, 2015

ckoerner added a commit to ckoerner/SemanticMediaWiki that referenced this issue Mar 20, 2015

JeroenDeDauw added a commit that referenced this issue Mar 22, 2015

Merge pull request #907 from jongfeli/master
Fix coding style issues #899

hermannschwaerzlerUIBK added a commit to hermannschwaerzlerUIBK/SemanticMediaWiki that referenced this issue Mar 23, 2015

JeroenDeDauw added a commit that referenced this issue Mar 23, 2015

Merge pull request #909 from hermannschwaerzlerUIBK/master
In reference to #899: add missing and remove unnecessary spacing

hermannschwaerzlerUIBK added a commit to hermannschwaerzlerUIBK/SemanticMediaWiki that referenced this issue Mar 23, 2015

mwjames added a commit that referenced this issue Mar 23, 2015

Merge pull request #903 from ckoerner/master
Fixing spacing and line length in a few files #899

JeroenDeDauw added a commit that referenced this issue Mar 23, 2015

Merge pull request #911 from hermannschwaerzlerUIBK/master
some more minor spacing changes wrt #899
@hermannschwaerzlerUIBK

This comment has been minimized.

Contributor

hermannschwaerzlerUIBK commented Mar 24, 2015

I was looking through the output of phpcs yesterday and fixed those issues that were easy to fix.
Doing this I found some that are not fixable IMO:

  • All method parameter ... is never used warnings for src/SPARQLStore/QueryEngine/RawResultParser.php:

    146 | WARNING | The method parameter $parser is never used
    156 | WARNING | The method parameter $parser is never used
    196 | WARNING | The method parameter $parser is never used
    196 | WARNING | The method parameter $elementTag is never used
    203 | WARNING | The method parameter $parser is never used

    All those functions are xml-parser callbacks and have to have that second parameter to be valid.

  • The errors regarding empty IF/ELSEIF statements in

  • src/SPARQLStore/QueryEngine/ConditionBuilder/ConjunctionConditionBuilder.php (162)

  • src/SPARQLStore/QueryEngine/ConditionBuilder/DisjunctionConditionBuilder.php (163)

  • src/SPARQLStore/QueryEngine/QueryEngine.php (278)

  • src/SQLStore/QueryEngine/Compiler/SomePropertyCompiler.php (211).

Those contain a comment why they are intentionally left empty.

What's the best way to handle these noise generating parts?
Shall we put // @codingStandardsIgnoreStart - // @codingStandardsIgnoreEnd comments around them?

@JeroenDeDauw

This comment has been minimized.

Member

JeroenDeDauw commented Mar 25, 2015

@hermannschwaerzlerUIBK I checked the if/elseif stuff, and all of those are indeed better left in the code for clarity.

I'm fine with adding @codingStandardsIgnoreStart tags, though for some categories such as unused parameters, it's probably better to just ignore them. We might want to exclude whole such rules then by default. Alternatively, you can run PHPCS with a single rule, and thus not be bothered by the noise of others. I've added some additional instructions in the first post.

JeroenDeDauw added a commit that referenced this issue Mar 25, 2015

Merge pull request #921 from jongfeli/master
Fix coding style issues #899

JeroenDeDauw added a commit that referenced this issue Mar 26, 2015

Merge pull request #923 from jongfeli/master
Fix coding style issues #899

mwjames added a commit that referenced this issue Mar 27, 2015

Merge pull request #925 from jongfeli/master
Fix coding style issues #899
@jongfeli

This comment has been minimized.

Contributor

jongfeli commented Mar 28, 2015

In includes/ there are a lot of Generic.ControlStructures.InlineControlStructure.NotAllowed violations like below in [includes/datavalues/SMW_DV_Property.php] line 91.

    /**
     * We use the internal wikipage object to store some of this objects data.
     * Clone it to make sure that data can be modified independently from the
     * original object's content.
     */
    public function __clone() {
        if ( !is_null( $this->m_wikipage ) ) $this->m_wikipage = clone $this->m_wikipage;
    }

Is it fixed and ok if written like this:

    /**
     * We use the internal wikipage object to store some of this objects data.
     * Clone it to make sure that data can be modified independently from the
     * original object's content.
     */
    public function __clone() {
        if ( !is_null( $this->m_wikipage ) ) {
            $this->m_wikipage = clone $this->m_wikipage;
        }
    }

Or like this:

    /**
     * We use the internal wikipage object to store some of this objects data.
     * Clone it to make sure that data can be modified independently from the
     * original object's content.
     */
    public function __clone() {
        if ( !is_null( $this->m_wikipage ) ) {
            $this->m_wikipage = clone;
            $this->m_wikipage;
        }
    }
@joelkp

This comment has been minimized.

Contributor

joelkp commented Mar 28, 2015

It's the first of those two options.

JeroenDeDauw added a commit that referenced this issue Mar 29, 2015

Merge pull request #931 from SemanticMediaWiki/teststyle
Fix coding style issues #899 in tests/
@JeroenDeDauw

This comment has been minimized.

Member

JeroenDeDauw commented Mar 29, 2015

Oh yeah, indeed the first one. If you have an if or while or for or whatever thing as follows

if ( some condition ) doSomeStuff 
while ( some condition ) doSomeStuff

It should be changed to this format

if ( some condition ) {
    doSomeStuff
}

JeroenDeDauw added a commit that referenced this issue Apr 1, 2015

Merge pull request #949 from jongfeli/master
Fix coding style issues #899

JeroenDeDauw added a commit that referenced this issue Apr 2, 2015

Merge pull request #955 from jongfeli/master
Fix coding style issues #899
@JeroenDeDauw

This comment has been minimized.

Member

JeroenDeDauw commented Apr 2, 2015

I've updated the cs-standalone command to now include includes/ and tests/. #956

JeroenDeDauw added a commit that referenced this issue Apr 3, 2015

Merge pull request #957 from jongfeli/master
Fix coding style issues #899 src/ includes/

JeroenDeDauw added a commit that referenced this issue Apr 4, 2015

Merge pull request #966 from jongfeli/Code-violations
Fixing various coding style issues #899

JeroenDeDauw added a commit that referenced this issue Apr 5, 2015

JeroenDeDauw added a commit that referenced this issue Sep 29, 2015

JeroenDeDauw added a commit that referenced this issue Sep 30, 2015

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Oct 6, 2015

JeroenDeDauw added a commit that referenced this issue Oct 7, 2015

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Nov 1, 2015

mwjames added a commit that referenced this issue Nov 1, 2015

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Nov 14, 2015

JeroenDeDauw added a commit that referenced this issue Nov 14, 2015

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Jan 3, 2016

JeroenDeDauw added a commit that referenced this issue Jan 3, 2016

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Jun 25, 2016

JeroenDeDauw added a commit that referenced this issue Jun 25, 2016

@mwjames mwjames modified the milestones: SMW 2.5, SMW 2.4 Jul 9, 2016

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Sep 17, 2016

@kghbln kghbln modified the milestones: SMW 3.0, SMW 2.5.0 Mar 6, 2017

jongfeli added a commit to jongfeli/SemanticMediaWiki that referenced this issue Jun 4, 2017

@JeroenDeDauw JeroenDeDauw removed this from the SMW 3.0.0 milestone Sep 3, 2018

@kghbln kghbln changed the title from Fix coding style issues to [Tracking ticket] Fix coding style issues Sep 18, 2018

@kghbln kghbln added this to the SMW 3.1.0 milestone Sep 18, 2018

@kghbln

This comment has been minimized.

Member

kghbln commented Sep 18, 2018

A lot of improvements were done here thanks to numerous contributors. This can be done again once in a while.

This issue been categorized and selected as "Enhancements and features" and since it has not seen any update within the last 90 days, it has now been closed and moved to the "Enhancements and features" board. In case the issue finds a sponsor or is actively worked on it can be reopened.

@kghbln kghbln closed this Sep 18, 2018

@kghbln kghbln moved this from Open to Tracking in Enhancements and features Sep 18, 2018

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