Fix typos in comments, add phpdocs, replace NBSP by space, simplify phpunit #1142

kinow · 2021-03-22T20:40:41Z

Happy to drop some commits if necessary 👍 Just syncing my local fork branch and taking a quick look at the current code base.

sonarqubecloud · 2021-03-22T20:41:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
2.0% Duplication

controller/Controller.php

model/Concept.php

kinow · 2021-03-22T20:43:16Z

model/resolver/WDQSResource.php

@@ -6,7 +6,6 @@ class WDQSResource extends RemoteResource
    const WDQS_ENDPOINT = "https://query.wikidata.org/sparql";

    public function resolve(int $timeout) : ?EasyRdf\Resource {
-        $client = new EasyRdf\Sparql\Client(self::WDQS_ENDPOINT);


Re-declared in the code below

Good catch!

Now that I look at it, it is a bit questionable why lines

Skosmos/model/resolver/WDQSResource.php

Lines 10 to 44 in 4774577

// unregister the legacy "json" format as it causes problems with CONSTRUCT requests

EasyRdf\Format::unregister('json');

// change the timeout setting for external requests

$httpclient = EasyRdf\Http::getDefaultHttpClient();

$httpclient->setConfig(array('timeout' => $timeout));

$httpclient->setHeaders('Accept', 'text/turtle');

EasyRdf\Http::setDefaultHttpClient($httpclient);

$uri = $this->uri;

$query = <<<EOQ

PREFIX wd: <http://www.wikidata.org/entity/>

PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#>

PREFIX schema: <http://schema.org/>

CONSTRUCT {

<$uri> rdfs:label ?label .

?link schema:about <$uri> ;

a ?linktype ;

schema:isPartOf ?whole ;

schema:inLanguage ?lang .

}

WHERE

{

{ <$uri> rdfs:label ?label }

UNION

{

?link schema:about <$uri> ;

a ?linktype ;

schema:isPartOf ?whole ;

schema:inLanguage ?lang .

}

}

EOQ;

$client = new EasyRdf\Sparql\Client(self::WDQS_ENDPOINT);

are in the try block, though. I would expect them to succeed in any case.

Hmm, good point. We can move it out maybe in another PR? One thing that really bothers me in the code is the use of return $this->showError… functions, while these showError-etc functions actually do not return anything; but instead set HTTP header() values, print to the stdout with echo() and need the stream to be closed (with return or exit) somewhere.

e.g.

Skosmos/controller/RestController.php

Line 49 in c84dc97

return $this->returnError(400, "Bad Request", "Invalid limit parameter");

returns the value of

Skosmos/controller/Controller.php

Lines 179 to 184 in c84dc97

protected function returnError($code, $status, $message)

{

header("HTTP/1.0 $code $status");

header("Content-type: text/plain; charset=utf-8");

echo "$code $status : $message";

}

(which doesn't actually return anything).

I started fixing that in this PR, but the change was getting quite big and I would need to do some manual testing to confirm it didn't break anything 😓

Yes, best to leave it to another PR, this one is rock solid as it is! 😃

Heh, now that you pointed at it, it surely looks ugly. I also remember having some trouble with those things whilst trying to silence the extra output PHPUnit was printing whilst executing tests.

+1 for another PR.

codecov · 2021-03-22T20:45:27Z

Codecov Report

Merging #1142 (4774577) into master (c84dc97) will decrease coverage by 0.00%.
The diff coverage is 55.55%.

@@             Coverage Diff              @@
##             master    #1142      +/-   ##
============================================
- Coverage     67.96%   67.96%   -0.01%     
  Complexity     1584     1584              
============================================
  Files            32       32              
  Lines          3890     3889       -1     
============================================
- Hits           2644     2643       -1     
  Misses         1246     1246

Impacted Files	Coverage Δ	Complexity Δ
controller/Controller.php	`57.36% <ø> (ø)`	`54.00 <0.00> (ø)`
controller/RestController.php	`13.02% <0.00%> (ø)`	`194.00 <0.00> (ø)`
controller/WebController.php	`16.87% <ø> (ø)`	`84.00 <0.00> (ø)`
model/Model.php	`83.68% <ø> (ø)`	`105.00 <0.00> (ø)`
model/Request.php	`67.85% <ø> (ø)`	`51.00 <0.00> (ø)`
model/VocabularyConfig.php	`94.00% <ø> (ø)`	`89.00 <0.00> (ø)`
model/resolver/WDQSResource.php	`94.44% <ø> (-0.30%)`	`3.00 <0.00> (ø)`
model/sparql/GenericSparql.php	`92.12% <ø> (ø)`	`319.00 <0.00> (ø)`
model/Concept.php	`80.95% <50.00%> (ø)`	`197.00 <0.00> (ø)`
model/BaseConfig.php	`66.66% <100.00%> (ø)`	`8.00 <0.00> (ø)`
... and 1 more

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 c84dc97...4774577. Read the comment docs.

kouralex · 2021-03-22T21:34:27Z

controller/WebController.php

-    * @param string $lang the language for displaying dates in the change list
-    */
+     * Formats the list of concepts as labels arranged by modification month
+     * @param Array $changeList


It seems like there are PHP types stated in both uppercase and lowercase (starting with) in PHPDocs all over the code base (array, for instance, here).

Well, I guess they do not matter. 😅

I noticed that too. I think in this case I opened another code in this function, that returned Array, and then I just typed Array too 😄 but normally I use string, boolean, array… happy to change it to lower case if preferred.

I guess it is OK at this point.

We should anyways add type hints/declarations directly into function arguments and return values as requested in #727 but that can be done en masse, simultaneously double checking these PHPDocs.

kouralex · 2021-03-22T21:50:25Z

tests/GenericSparqlTest.php

@@ -1051,7 +1051,7 @@ public function testListConceptGroups()
    $voc = $this->model->getVocabulary('groups');
    $graph = $voc->getGraph();
    $sparql = new GenericSparql('http://localhost:13030/skosmos-test/sparql', $graph, $this->model);
-    $actual = $sparql->ListConceptGroups('http://www.w3.org/2004/02/skos/core#Collection', 'en', false);
+    $actual = $sparql->ListConceptGroups('http://www.w3.org/2004/02/skos/core#Collection', 'en');


Good fix!

Oh my, PHP truly is case-insensitive. Hurts my eyes! 😄

kouralex

Very solid improvements in this PR, thanks @kinow !

I am more than happy to merge this as-is.

kinow · 2021-03-22T22:50:13Z

Thanks for the review and helpful comments @kouralex !

kouralex · 2021-03-23T18:02:31Z

I merged this now that I saw your last replies. 🎉

kinow added 9 commits March 23, 2021 09:08

Fix few minor typos

727f0ba

Replace NBSP by space

27b3ec9

Remove unnecessary $client var (re-declared further down)

00b06b8

Use PHPUnit assertions methods

ae1ec27

Remove dangling semi-colons

63a5cbd

Remove extra parameters (not specified in function declaration)

dc868e4

Replace NBSP non-blank space by space

09c1f78

Fix type declaration in phpdocs

3202ddb

Add @return phpdoc tags, remove unnecessary type definition var

4774577

kinow commented Mar 22, 2021

View reviewed changes

kouralex reviewed Mar 22, 2021

View reviewed changes

kouralex approved these changes Mar 22, 2021

View reviewed changes

kouralex merged commit d904172 into NatLibFi:master Mar 23, 2021

kouralex added this to the 2.10 milestone Mar 23, 2021

kinow deleted the typos branch March 23, 2021 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typos in comments, add phpdocs, replace NBSP by space, simplify phpunit #1142

Fix typos in comments, add phpdocs, replace NBSP by space, simplify phpunit #1142

kinow commented Mar 22, 2021 •

edited

Loading

sonarqubecloud bot commented Mar 22, 2021

kinow Mar 22, 2021

kouralex Mar 22, 2021

kinow Mar 22, 2021 •

edited

Loading

kouralex Mar 22, 2021

codecov bot commented Mar 22, 2021 •

edited

Loading

kouralex Mar 22, 2021

kinow Mar 22, 2021 •

edited

Loading

kouralex Mar 22, 2021

kouralex Mar 22, 2021

kouralex left a comment

kinow commented Mar 22, 2021

kouralex commented Mar 23, 2021

	// unregister the legacy "json" format as it causes problems with CONSTRUCT requests
	EasyRdf\Format::unregister('json');
	// change the timeout setting for external requests
	$httpclient = EasyRdf\Http::getDefaultHttpClient();
	$httpclient->setConfig(array('timeout' => $timeout));
	$httpclient->setHeaders('Accept', 'text/turtle');
	EasyRdf\Http::setDefaultHttpClient($httpclient);

	$uri = $this->uri;
	$query = <<<EOQ
	PREFIX wd: <http://www.wikidata.org/entity/>
	PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#>
	PREFIX schema: <http://schema.org/>

	CONSTRUCT {
	<$uri> rdfs:label ?label .
	?link schema:about <$uri> ;
	a ?linktype ;
	schema:isPartOf ?whole ;
	schema:inLanguage ?lang .
	}
	WHERE
	{
	{ <$uri> rdfs:label ?label }
	UNION
	{
	?link schema:about <$uri> ;
	a ?linktype ;
	schema:isPartOf ?whole ;
	schema:inLanguage ?lang .
	}
	}
	EOQ;

	$client = new EasyRdf\Sparql\Client(self::WDQS_ENDPOINT);

	protected function returnError($code, $status, $message)
	{
	header("HTTP/1.0 $code $status");
	header("Content-type: text/plain; charset=utf-8");
	echo "$code $status : $message";
	}

Fix typos in comments, add phpdocs, replace NBSP by space, simplify phpunit #1142

Fix typos in comments, add phpdocs, replace NBSP by space, simplify phpunit #1142

Conversation

kinow commented Mar 22, 2021 • edited Loading

sonarqubecloud bot commented Mar 22, 2021

kinow Mar 22, 2021

Choose a reason for hiding this comment

kouralex Mar 22, 2021

Choose a reason for hiding this comment

kinow Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

kouralex Mar 22, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2021 • edited Loading

Codecov Report

kouralex Mar 22, 2021

Choose a reason for hiding this comment

kinow Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

kouralex Mar 22, 2021

Choose a reason for hiding this comment

kouralex Mar 22, 2021

Choose a reason for hiding this comment

kouralex left a comment

Choose a reason for hiding this comment

kinow commented Mar 22, 2021

kouralex commented Mar 23, 2021

kinow commented Mar 22, 2021 •

edited

Loading

kinow Mar 22, 2021 •

edited

Loading

codecov bot commented Mar 22, 2021 •

edited

Loading

kinow Mar 22, 2021 •

edited

Loading