Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] Standardize exception definitions #12583

Merged
merged 5 commits into from
Aug 9, 2018

Conversation

friedbunny
Copy link
Contributor

Fixes #7258 by converting stringly-typed exception names into MGLExceptionName-typed constants. This doesn’t attempt to document the exceptions, but that could happen in the future.

/cc @1ec5 @fabian-guerra @julianrex @captainbarbosa

@friedbunny friedbunny added iOS Mapbox Maps SDK for iOS refactor needs changelog Indicates PR needs a changelog entry prior to merging. labels Aug 8, 2018
@friedbunny friedbunny self-assigned this Aug 8, 2018
Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

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

Great to see these changes!
The only comment I have is: should the exception names be of the form MGLException<Name>? So MGLExceptionAbstractClass instead of MGLAbstractClassException?

@friedbunny
Copy link
Contributor Author

friedbunny commented Aug 8, 2018

Should the exception names be of the form MGLException<Name>? So MGLExceptionAbstractClass instead of MGLAbstractClassException?

MGLFooException follows Cocoa’s naming convention for exceptions and is how Foundation names its exceptions.

(This makes sense to me when I think of it in terms of calling our UIView subclass containing a map MGLMapView instead of MGLViewMap.)

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

Instead of having a master header with all the exceptions, we will declare on each file the exceptions it throws (if any)? If so, should we document this?

@friedbunny friedbunny added this to the ios-v4.4.0 milestone Aug 8, 2018
@friedbunny
Copy link
Contributor Author

friedbunny commented Aug 8, 2018

@julianrex and I were curious about how this ended up looking in Swift, so:

screen shot 2018-08-08 at 6 58 44 pm

@@ -30,6 +30,11 @@ NS_ASSUME_NONNULL_BEGIN
typedef NSString *NSNotificationName;
#endif

typedef NSString *MGLExceptionName NS_EXTENSIBLE_STRING_ENUM;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was copying NSExceptionName, but I’m going to look at replacing this with NS_TYPED_EXTENSIBLE_ENUM on Apple’s recommendation:

You might encounter Objective-C code that uses the older NS_STRING_ENUM and NS_EXTENSIBLE_STRING_ENUM macros, which were used to group string constants. Use NS_TYPED_ENUM and NS_TYPED_EXTENSIBLE_ENUM when grouping related constants of any type, including string constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Xcode 9 has all of these defined as the same thing, so it’s just cosmetic:

#define NS_STRING_ENUM _NS_TYPED_ENUM
#define NS_EXTENSIBLE_STRING_ENUM _NS_TYPED_EXTENSIBLE_ENUM

#define NS_TYPED_ENUM _NS_TYPED_ENUM
#define NS_TYPED_EXTENSIBLE_ENUM _NS_TYPED_EXTENSIBLE_ENUM

... but let’s still standardize on the new one.

@friedbunny
Copy link
Contributor Author

Instead of having a master header with all the exceptions, we will declare on each file the exceptions it throws (if any)? If so, should we document this?

Defining non-generic constants close to their main usage sites is generally a good practice, I think, and is something we’ve been doing with other constants, like MGLTileSourceOption and friends. I’ll go through and add a bit of private documentation in MGLTypes to try and make it clear that those are generic exceptions.

@friedbunny friedbunny force-pushed the fb-exception-standardization branch from eae1570 to 2c18acb Compare August 9, 2018 16:39
@friedbunny friedbunny removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 9, 2018
@friedbunny friedbunny merged commit ec7b920 into master Aug 9, 2018
@friedbunny friedbunny deleted the fb-exception-standardization branch August 9, 2018 17:35
@@ -6296,8 +6301,8 @@ - (void)setStyleURL__:(nullable NSString *)URLString
NSURL *url = URLString.length ? [NSURL URLWithString:URLString] : nil;
if (URLString.length && !url)
{
[NSException raise:@"Invalid style URL" format:
@"“%@” is not a valid style URL.", URLString];
[NSException raise:MGLInvalidStyleURLException
Copy link
Contributor

Choose a reason for hiding this comment

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

#12640 takes care of the macOS side of this change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants