-
Notifications
You must be signed in to change notification settings - Fork 5
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
full code review #44
full code review #44
Conversation
…kidata-lib/PropertySuggester into SuggestionsByPrefix
…/property-suggester into modify_entity_selector_from_extension fix whitespace and some code improvements
…th paramter "nosuggestions"
…extension Modify entity selector from extension to show suggestions
remove hack for empty entity
remove "compressed" csv format
Small fix: Only show aliases if available
…tupels Use entity and claim objects instead of tupels
…gester into apply_coding_guidelines
rename createJson to createResultArray
public function generateSuggestionsByPropertyList( array $propertyList, $limit, $minProbability ) { | ||
$properties = array(); | ||
foreach ( $propertyList as $id ) { | ||
$properties[] = PropertyId::newFromNumber( $this->getNumericPropertyId( $id ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you just want to do new PropertyId( $stringId )
The var names $properties
and $propertyList
are bad. They contain instances of PropertyId and property id strings, respectively.
…inProbability, remove mysqlimporter
fix review issues 2
fix ResultSize + fix naming + strategy in generateSuggestionsByPropertyList
remove condition for autoloader, secure $minProbability
Fix packagist links in readme
If we install property suggester via composer, by including it in the Wikidata "build", in MediaWiki's composer.json or other such setup then autoload.php will be elsewhere and handled elsewhere.
Check that vendor/autoload.php exists
Best to make the description general without specific mention of Wikidata.
Improve extension description
fix qualifier were counted as mainsnak
Same hack used in Wikibase, ValueView, etc. to allow this to work if an extension is installed in a non-standard place.
Fix remoteExtPath for installation in non-standard locations
parent::__construct(); | ||
$this->mDescription = "Read CSV Dump and refill probability table"; | ||
$this->addOption( 'file', 'CSV table to be loaded (relative path)', true, true ); | ||
$this->setBatchSize( 10000 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1000 would be more appropriate. The current value might cause some slave lag due to having too many rows at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently use 10000 as suggested here: https://bugzilla.wikimedia.org/show_bug.cgi?id=63224#c5
Should it be reduced to 1000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably. Of course it always be set at run-time, but 1000 seem like a safer default (and doesn't lose much speed vs 10000).
No description provided.