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

Use appropriate part of speech for properties #7457

Merged
merged 7 commits into from
Dec 16, 2016

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Dec 15, 2016

Fixed overridden property references in requirements lists. Boolean-typed properties can now have getters beginning with “is”.

Renamed a number of layout properties according to the following rules, synopsized from the Cocoa naming guidelines for properties:

  1. Boolean-typed properties should read like predicates (and include a verb).
  2. Other properties must be noun phrases.
  3. All properties must be grammatical.

In most cases, you can get the right getter or setter in Xcode’s code completion popup by typing part of the style specification name for the property. For example, typing allowover brings up both iconAllowsOverlap and setIconAllowsOverlap:.

The main impetus for these changes is that property getters and action methods can look identical in Objective-C: [layer fillAntialias] looks like a method that performs an action but returns no value. The new syntax, [layer isFillAntialiased], is unambiguous. Meanwhile, dot syntax is unaffected: layer.fillAntialiased.

Here are the changes:

Old getter New getter Old setter New setter
-fillAntialias -isFillAntialiased -setFillAntialias: -setFillAntialiased:
-iconAllowOverlap -iconAllowsOverlap -setIconAllowOverlap: -setIconAllowsOverlap:
-iconIgnorePlacement -iconIgnoresPlacement -setIconIgnorePlacement: -setIconIgnoresPlacement:
-iconKeepUpright -keepsIconUpright -setIconKeepUpright: -setKeepsIconUpright:
-iconOptional -isIconOptional -setIconOptional: -setIconOptional:
-iconRotate -iconRotation -setIconRotate: -setIconRotation:
-symbolAvoidEdges -symbolAvoidsEdges -setSymbolAvoidEdges: -setSymbolAvoidsEdges:
-textAllowOverlap -textAllowsOverlap -setTextAllowOverlap: -setTextAllowsOverlap:
-textIgnorePlacement -textIgnoresPlacement -setTextIgnorePlacement: -setTextIgnoresPlacement:
-textJustify -textJustification -setTextJustify: -setTextJustification:
-textKeepUpright -keepsTextUpright -setTextKeepUpright: -setKeepsTextUpright:
-textMaxAngle -maximumTextAngle -setTextMaxAngle: -setMaximumTextAngle:
-textMaxWidth -maximumTextWidth -setTextMaxWidth: -setMaximumTextWidth:
-textOptional -isTextOptional -setTextOptional: -setTextOptional:
-textRotate -textRotation -setTextRotate: -setTextRotation:

Before this PR can land:

  • Add the style specification names as aliases but mark them as unavailable
  • Implement getters for unavailable properties to avoid bloating the class with autosynthesized ivars that never get used
  • Add unit tests that enforce rules 1 and 2 using NSLinguisticTagSchemeLexicalClass

Working towards #6098.

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling labels Dec 15, 2016
@1ec5 1ec5 requested a review from frederoni December 15, 2016 18:36
@mention-bot
Copy link

@1ec5, thanks for your PR! By analyzing this pull request, we identified @boundsj, @incanus and @jfirebaugh to be potential reviewers.

@1ec5 1ec5 added this to the ios-v3.4.0 milestone Dec 15, 2016
1ec5 added 7 commits December 15, 2016 17:35
Fixed overridden property references in requirements lists. Boolean-typed properties can now have getters beginning with “is”.

Renamed a number of layout properties according to the following rules: Boolean-typed properties should include a verb; other properties must be noun phrases; all properties must be grammatical.
Renamed properties now have aliases based on their style specification names, marked unavailable, for wayfinding purposes.
Run property getter names through a basic battery of tests to see if they’re grammatical. Most part-of-speech tagging tests are guarded by a compile-time flag, off by default, because NSLinguisticTagger does a poor job of telling nouns from verbs, and we’ve intentionally kept many words in property names that could be read as either verbs or nouns (like “transform” or “scale”).
@1ec5 1ec5 force-pushed the 1ec5-verbing-weirds-properties-6098 branch from 0195052 to e5a13bc Compare December 16, 2016 01:35
@@ -6,5 +6,15 @@

@property (nonatomic) IBOutlet MGLMapView *mapView;
@property (nonatomic) XCTestExpectation *expectation;
@property (nonatomic, copy, readonly, class) NSString *layerType;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we dropped support for Xcode 7? Great use for the new class property. 👍 Does this mean it's safe to use not only in tests?

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 figured we only run the tests on Xcode 8, so there's no harm in using it here. If we decide to drop support for Xcode 7.3, that needs to be a more deliberate decision, and it's probably too close to the release to make that change. We just recently dropped support for Xcode 7.2 in v3.3.7.

@implementation NSString (MGLStyleLayerTestAdditions)

- (NS_ARRAY_OF(NSString *) *)lexicalClasses {
NSOrthography *orthography = [NSOrthography orthographyWithDominantScript:@"Latn"
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 fantastic. A nitpicker's dream come true ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this method doesn't get used for much by default. I put most calls to this method behind a compile-time flag that's off by default, because NSLinguisticTagging is actually pretty bad at tagging parts of speech for individual words. We could do better by moving these tests into JavaScript as part of the codegen script, but unfortunately the most popular npm tagging modules either require calls to a Web API or are licensed under the GPL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, when I turned on the stricter checks locally, they did catch a property I'd missed. So it's a useful tool to run once in awhile, even if there are many false positives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just occurred to me that we could try to form sentences around these property names and run them through grammar checking. Something to consider if we keep having to rename properties for grammar issues, I suppose.

@1ec5 1ec5 merged commit 3f26e88 into release-ios-v3.4.0 Dec 16, 2016
@1ec5 1ec5 deleted the 1ec5-verbing-weirds-properties-6098 branch December 16, 2016 16:09
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 23, 2016

mapbox/mapbox-gl-style-spec#201 would upstream most of these changes.

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

Successfully merging this pull request may close these issues.

3 participants