Skip to content

Conversation

@lohanidamodar
Copy link
Member

No description provided.

class {{spec.title | caseUcfirst}}Exception implements Exception {
final String message;
final int code;
final dynamic response;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we maybe have a specific type here? 🤔
String maybe?
Map<String, dynamic> is fine as well, but as a mobile developer who is coming from strongly typed languages like kotlin, java or swift I like having types 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

If the response can be different in some cases it's fine to have dynamic but then I would add a little comment to it. In our case, we are casting to some type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the response can be Json or can be String as well. @eldadfux do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh It might be tricky because sometimes a proxy can throw the error and we can't be sure of the format.

@lohanidamodar lohanidamodar marked this pull request as ready for review February 25, 2021 09:18
}
} on DioError catch(e) {
if(e.response == null) {
throw {{spec.title | caseUcfirst}}AppwriteException(e.message);
Copy link
Member

Choose a reason for hiding this comment

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

I think this line in the Exceptions PR is wrong: throw {{spec.title | caseUcfirst}}AppwriteException(e.message); as it resolve to AppwriteAppwriteException

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah !

@eldadfux eldadfux merged commit 7f7306a into appwrite:master Feb 25, 2021
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