Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Simplify DataType and DataTypeFactory #34

Merged
merged 1 commit into from
Aug 8, 2015

Conversation

brightbyte
Copy link

[BREAKING] This removes ValueValidators from DataType, which was unused.
DataType is no a simple pair of data type name and the associated value type id.

DataTypeFactory is accordingly simplified to no longer use callbacks, but instead
just take an array mapping data type names to value type ids.

Needed by https://gerrit.wikimedia.org/r/#/c/228848

This is a breaking change, since it modifies the constructor signature of DataType and DataTypeFactory.

@JeroenDeDauw
Copy link
Contributor

Can you squash those commits together so we do not have a broken one in the history?

}
],
"support": {
"irc": "irc://irc.freenode.net/wikidata"
},
"require": {
"php": ">=5.3.0"
"php": ">=5.3.0",
"wikimedia/assert": "0.2.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

~0.2.2

@JeroenDeDauw
Copy link
Contributor

-2 (or not, odd case...). I like the simplification being done here, and think the commits are fine apart from some small issues. However, given https://phabricator.wikimedia.org/T100426, I think it makes more sense to delete the code here and move it (or rather the new version) to Wikibase.git. Given the work being done, it seems rather misplaced at the moment. Once everything has settled down we can figure out where to best move it to.

*/
protected $validators;

/**
* @since 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Not correct any more.

@thiemowmde
Copy link
Contributor

README, version numbers and some @since tags should be changed. I also suggest to keep some methods for compatibility, at least the methods that still make sense without changing their parameters. Otherwise +1 for the patch in general.

This removes ValueValidators from DataType, which was unused.
DataType is no a simple pair of data type name and the associated value type id.

DataTypeFactory is accordingly simplified to no longer use callbacks, but instead
just take an array mapping data type names to value type ids.

Needed by https://gerrit.wikimedia.org/r/#/c/228848
@brightbyte
Copy link
Author

-2 (or not, odd case...). I like the simplification being done here, and think the commits are fine
apart from some small issues. However, given https://phabricator.wikimedia.org/T100426, I think it > makes more sense to delete the code here and move it (or rather the new version) to
Wikibase.git.

I think it makes more sense to clean it up here first, and then move

Given the work being done, it seems rather misplaced at the moment. Once everything has
settled down we can figure out where to best move it to.

I agree, but since I need this change for the refactoring of the boostrap code for formatters, parsers, and validators, I'd really like to have this releaded before we have figured out SWikiBaseServices.

@brightbyte
Copy link
Author

README, version numbers and some @SInCE tags should be changed. I also suggest to keep some
methods for compatibility, at least the methods that still make sense without changing their
parameters. Otherwise +1 for the patch in general.

Done, I think.

@JeroenDeDauw
Copy link
Contributor

I think it makes more sense to clean it up here first, and then move

Why? That's 2 breaking changes instead of 1.

I agree, but since I need this change for the refactoring of the boostrap code for formatters, parsers, and validators, I'd really like to have this releaded before we have figured out SWikiBaseServices.

As I said, we do not need to figure something difficult out first. We can just make the move. If you prefer, I'll happily submit the new version of this code against Wikibase.git and submit a deletion PR here. Only takes a few minutes.

@JeroenDeDauw
Copy link
Contributor

I had a go at moving this to WB.git and found that the JS binding is causing too much complications. So let's do that separately.

JeroenDeDauw added a commit that referenced this pull request Aug 8, 2015
Simplify DataType and DataTypeFactory
@JeroenDeDauw JeroenDeDauw merged commit 9c85512 into master Aug 8, 2015
@JeroenDeDauw JeroenDeDauw deleted the Simplify_DataTypeFactory branch August 8, 2015 07:53
### 0.5 (2015-08-06)

* Drop support for ValueValidators in DataTypes
* Simplified DataTypeFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

This is way to vague. This should at least be marked as "breaking changes".

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, these are not good release notes entries, as they do not tell the users of the component what they need to know. I've updated these lines shortly after merging this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen your edit after I wrote this. Thanks a lot!

brightbyte pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this pull request Aug 25, 2015
Requires new release of the DataTypes component,
see wmde/DataTypes#34

Change-Id: I23f35b0bda8f20f502bb5b981e8ad438f04516b5
public function __construct( array $typeBuilders = array() ) {
//TODO: check element type
$this->typeBuilders = $typeBuilders;
public function __construct( array $valueTypes ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The constructor parameter not being optional was a breaking change, not updated correctly everywhere. See #51.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
3 participants