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

refactor(image-source): throw if source is not a correct native instance #5273

Merged
merged 3 commits into from
Jan 23, 2018

Conversation

vchimev
Copy link
Contributor

@vchimev vchimev commented Jan 15, 2018

@ghost ghost assigned vchimev Jan 15, 2018
@ghost ghost added the in progress label Jan 15, 2018
@ns-bot
Copy link

ns-bot commented Jan 15, 2018

Please sign CLA at http://www.nativescript.org/cla

@ns-bot ns-bot added the cla: no label Jan 15, 2018
@ns-bot
Copy link

ns-bot commented Jan 15, 2018

CLA signature found, happy contributing!

Copy link

@DimitarTachev DimitarTachev left a comment

Choose a reason for hiding this comment

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

We could also think about removing the return value. Why do we need false when we pass null, instead of a simple void method? @PanayotCankov @hshristov

@@ -140,6 +140,9 @@ export class ImageSource implements ImageSourceDefinition {
}

public setNativeSource(source: any): boolean {
if (!(source instanceof android.graphics.Bitmap)) {

Choose a reason for hiding this comment

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

In this way, the method will no longer return false, it will throw an exception instead. It could be something like:

if (source && !(source instanceof android.graphics.Bitmap))

@@ -119,9 +119,10 @@ export class ImageSource implements ImageSourceDefinition {
}

public setNativeSource(source: any): boolean {
if (source instanceof UIImage) {
this.ios = source;
if (!(source instanceof UIImage)) {

Choose a reason for hiding this comment

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

In this way, the method will no longer return false, it will throw an exception instead. It could be something like:

if (source && !(source instanceof UIImage))

@vchimev
Copy link
Contributor Author

vchimev commented Jan 16, 2018

Thank you, @DimitarTachev!

public setNativeSource(source: any): boolean {
if (source instanceof UIImage) {
this.ios = source;
public setNativeSource(source: any): void {
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 a change in the public API -> mark this as breaking change if it is really needed.
Also the definitions (image-source.d.ts) should be updated

Copy link
Contributor Author

@vchimev vchimev Jan 19, 2018

Choose a reason for hiding this comment

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

Done, thanks.

BREAKING CHANGE:

Change the return type of `setNativeSource` method from `boolean` to `void`.
@vchimev
Copy link
Contributor Author

vchimev commented Jan 19, 2018

Changes are done, branch is rebased and able to merge after green CI.

@vchimev vchimev merged commit 58d61ca into master Jan 23, 2018
@vchimev vchimev deleted the vchimev/img-src branch January 23, 2018 14:57
@lock
Copy link

lock bot commented Aug 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants