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

Introduce MismatchingDataValueTypeException #7

Merged
merged 5 commits into from
Apr 7, 2014
Merged

Conversation

filbertkm
Copy link
Contributor

to be used by ValueFormatters

to be used by ValueFormatters
@filbertkm
Copy link
Contributor Author

@JeroenDeDauw @brightbyte

This might be generic enough to also be used for parsers. If so, I am not quite sure where we should put this?

@JeroenDeDauw
Copy link
Member

Where do you intend to throw this? In the format method of ValueFormatter implementations? If so, then it should derive from the base formatting exception.

I just noticed a pile of fail with regard to that exception though. It is not referred to in the interface and is in the wrong namespace. That definitely needs to be fixed

@filbertkm
Copy link
Contributor Author

we plan to use this for ValueFormatters. I can have it derive from the base formatting exception and maybe fix the fail also :)

* @param string $message
* @param \Exception $previous
*/
public function __construct( $expectedValueType, $dataValueType, $message = '',

Choose a reason for hiding this comment

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

Please construct a meaningful $message from $expectedValueType and $dataValueType if not given (perhaps don't even allow the $mesage to be given explicitly, or just include the explicit one in the auto-generated one).

@filbertkm
Copy link
Contributor Author

renamed the exception and made the message more informative.

I had this extend InvalidArgumentException (somewhat agree) in an earlier patch but it's also desired to extend FormattingException (which is a RuntimeException). Being php, suppose we can't do everything here.

$this->expectedValueType = $expectedValueType;
$this->dataValueType = $dataValueType;

$message = "DataValueType $dataValueType does not match expected value "
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like DataValueType is a class.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. I'd just drop the DataValueType type part, not like repeating that is helping clarity anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MismatchingException? MismatchingDataValueException? BadDataValueTypeException as daniel suggests? or what?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like you are misplacing some of your comments...

@filbertkm
Copy link
Contributor Author

@thiemowmde what should we call it instead of DataValueType?

* @licence GNU GPL v2+
* @author Katie Filbert < aude.wiki@gmail.com >
*/
class DataValueTypeMismatchException extends FormattingException {
Copy link
Member

Choose a reason for hiding this comment

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

MismatchingDataValueTypeException? The current one is in the format of ArgumentInvalidException, which is odd

@filbertkm filbertkm changed the title Introduce DataValueMismatchException Introduce MismatchingDataValueTypeException Apr 7, 2014
JeroenDeDauw added a commit that referenced this pull request Apr 7, 2014
Introduce MismatchingDataValueTypeException
@JeroenDeDauw JeroenDeDauw merged commit 71b756c into master Apr 7, 2014
@JeroenDeDauw JeroenDeDauw deleted the mismatch branch April 7, 2014 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants