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

Remove Out-commented Code in Erfurt #140

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

shinobu
Copy link
Contributor

@shinobu shinobu commented Dec 17, 2016

This Pull Request removes all (or most) of the outcommented code in erfurt.

This Pull Request is based on #138 , which already started removing outcommented code in one file, so this request should be merged after it.

white-gecko and others added 2 commits November 11, 2016 15:14
Remove commented out code and apply coding standards of
Erfurt_Store_Adapter_EfZendDb
@shinobu
Copy link
Contributor Author

shinobu commented Jan 6, 2017

Tests:
Unit:
Tests: 672, Assertions: 1350, Incomplete: 3, Skipped: 1. (common 4 ignored ones)
Integration-Virtuoso:
Tests: 68, Assertions: 224, Skipped: 14. (14 zenddb tests, commonly ignored)
mysql:
Tests: 68, Assertions: 5834, Skipped: 12. (virtuoso tests same as above)

Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

But blocked by #138

@@ -268,10 +174,7 @@ public function queryModel(Erfurt_Sparql_Query $query, $resultform = 'plain')

$this->ts = new Erfurt_Sparql_EngineDb_TypeSorter($this->query, $this);

$this->_setOptions();

Copy link
Member

Choose a reason for hiding this comment

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

This method is removed from Line 367

//$this->setOffset(0);
//$this->order = new Erfurt_Sparql_Query2_OrderClause();
//$this->distinctReducedMode = 0;
//$this->projectionVars = array();
break;
case self::typeDescribe:
break;
Copy link
Member

Choose a reason for hiding this comment

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

Remove the breaks for select and ask, such that select, ask and describe result in one common break, (still they should not go to default)

Copy link
Member

Choose a reason for hiding this comment

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

Ok this is a change request ;-)

# 'st' => $subjectIs,
# 'ot' => $objectIs
#);

Copy link
Member

Choose a reason for hiding this comment

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

Here we have the same problem as in #138

# 'st' => $subjectIs,
# 'ot' => $objectIs
#);

Copy link
Member

Choose a reason for hiding this comment

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

This change comes from #138

Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

Sorry there was one change request

//$this->setOffset(0);
//$this->order = new Erfurt_Sparql_Query2_OrderClause();
//$this->distinctReducedMode = 0;
//$this->projectionVars = array();
break;
case self::typeDescribe:
break;
Copy link
Member

Choose a reason for hiding this comment

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

Ok this is a change request ;-)

@white-gecko
Copy link
Member

Please also run tests for MySQL backend

@shinobu shinobu force-pushed the feature/removeOutcommentedCode branch from 979de57 to 3d5e8b0 Compare January 6, 2017 19:43
@shinobu
Copy link
Contributor Author

shinobu commented Jan 6, 2017

done @white-gecko

…ommentedCode

Conflicts:
	library/Erfurt/Sparql/Query2.php
	library/Erfurt/Store/Adapter/Mssql.php
	library/Erfurt/Syntax/RdfParser/Adapter/RdfXml.php
Copy link
Contributor

@pfrischmuth pfrischmuth left a comment

Choose a reason for hiding this comment

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

just want to get this out of my inbox... sorry

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.

3 participants