Simplification, Clarification, Robotification #118

designatednerd · 2018-04-25T13:43:53Z

Per discussion yesterday, I took a swing through the Handbook to see what could be updated and simplified.

In this PR, I:

Broke out the Style Guide from the Best Practices. Basically, now the style guide contains requirements, and best practices are strong suggestions and links to things that you should or should not do based on other people's experience.
Added a default .swiftlint.yml file that should be usable across most projects immediately.
Condensed a couple of files which had been separate into other sections.
Made a number of points significantly more concise.
Removed a couple of things which a) Were overly prescriptive and/or b) Were not actually being followed.
Made an "Archive" folder and added a couple of files that hadn't been updated in over a year.

Feedback requested:

Is there anything in Style Guide that should be in Best Practices, or vice versa?
Is there anything you feel we're enforcing too aggressively with SwiftLint?
Is there anything you feel we should be enforcing too leniently with SwiftLint?
Is there anything here you feel was taken out too hastily?

Any other feedback is most welcome!

elland · 2018-04-25T13:46:01Z

COMMUNICATION.md

-It good practice to elaborate. It may seem inefficient to be so verbose in our replies, but it will avoid misunderstandings, which can result in way greater inefficiency.
+When you write a reply of less than ~5 words, have an alarm bell in the back of your head 🔔. Is it just tacit agreement? Then it’s probably fine! “Great job”, “Couldn’t have put it better”,  “totes awesome!” are all fine. 
+
+In every other case though: 🚨! It good practice to elaborate. It may seem inefficient to be so verbose in our replies, but it will avoid misunderstandings, which can result in far greater inefficiency.


It's, missed a verb :)

elland · 2018-04-25T13:46:45Z

COMMUNICATION.md


+- **I see you are using an optional as an array of User object in the example above**, have you considered to have that property default to an empty array?


to have -> having :)

onmyway133 · 2018-04-26T11:22:13Z

COMMUNICATION.md

@@ -1,51 +1,55 @@
-# Communicating within the iOS team
+# Communicating With Your Colleagues


I find this http://titlecase.com/ to be handy, so we should not uppercase unimportant words. What do you think @designatednerd ?

I'd avoid "title case" altogether, and just use good ol' regular spelling rules. First word of the sentence and proper nouns only. 😀

Yeah I think I did this a bit automatically as this is how I'm used to setting up titles in markdown documents. Will update.

onmyway133 · 2018-04-26T11:22:54Z

GIT_AND_GITHUB.md

@@ -4,20 +4,13 @@ These conventions apply to our open-source, internal, and client projects.

 ## Commits

-The `master` branch is the stable branch. It's also used for development
-purposes, as to avoid having `dev`, `development`, `staging`, `wip` and others.
+The `master` branch is the stable branch. It's also used for development purposes, as to avoid having `dev`, `development`, `staging`, `wip` and others.


marijnschilling · 2018-04-26T11:40:05Z

STYLE_GUIDE.md

@@ -0,0 +1,383 @@
+# Swift Style Guide


Did you add this whole thing, or did you just extract it from somewhere else? I wonder if there's any changes made to this.

I extracted it and condensed it from Best Practices. There are definitely changes made to it to try and make it more concise and closer to enforcing what we're already doing.

marijnschilling · 2018-04-26T11:40:30Z

REUSABLE_COMPONENTS.md

- [ ] Send a tweet to [@maniacdev](https://twitter.com/maniacdev)
- [ ] Send a tweet to [@daveverwer](https://twitter.com/daveverwer) and [@iOSDevWeekly](https://twitter.com/iOSDevWeekly)
- [ ] Send a tweet to [@iOSGoodies](https://twitter.com/iOSGoodies) with the link to your PR
- [ ] Send a tweet to [@NatashaTheRobot](https://twitter.com/NatashaTheRobot)


😭 But yeah, she quit :(

So did Maniac Dev - they haven't posted anything since late last year, even on their twitter.

…rate table of contents

# Conflicts: # BEST_PRACTICES.md

roberthein

I think this is a great improvement, very well written too. 👌

roberthein · 2018-05-02T10:32:18Z

COMMUNICATION.md

@@ -1,51 +1,62 @@
-# Communicating within the iOS team
+# Communicating with your colleaugues


One "u" too many. ;)

🤦‍♀️

elland · 2018-05-03T08:50:09Z

COMMUNICATION.md

+
+In every other case though: 🚨! It's good practice to elaborate. It may seem inefficient to be so verbose in our replies, but it will avoid misunderstandings, which can result in far greater inefficiency.
+
+## Criticizing or asking questions


👀 what happened here?

🇺🇸 spelling to be internally consistent with everything I was rewriting

marijnschilling

Thanks for picking this up Ellen. I think it's great when we all agree what is in here 🙏

i left some comments about what could be clearer. But there's nothing in here that i disagree with!

I would like it though if we could add a rule about always giving a description or screenshot/gif with a PR. I think it saves valuable time for the reviewer if the requester makes clear what they worked on, how obvious it might seem to them! (the thing is the more obvious it is the easier it is to make a description, so everybody wins)

But i think we have this in danger, but i don't always see this giving a warning when there's no description 🤔 Or is it disabled?

marijnschilling · 2018-05-03T09:08:49Z

default.swiftlint.yml

+  - override_in_extension # Extensions shouldn't override declarations.
+  - private_action # IBActions should be private.
+  - private_outlet # IBOutlets should be private to avoid leaking UIKit to higher layers.
+  - private_unit_test # Unit tests marked private are silently skipped.


Does this rule make sure we cannot mark tests as private because they are silently skipped when they are? Maybe clearer to word it like Unit tests should not be marked private because they are silently skipped

You are correct. And I copy pasted this description from the list of SwiftLint rules, but I'll update it.

marijnschilling · 2018-05-03T09:10:08Z

default.swiftlint.yml

+  - prohibited_super_call # Some methods should not call super
+  - redundant_nil_coalescing # nil coalescing operator is only evaluated if the lhs is nil, coalescing operator with nil as rhs is redundant
+  - switch_case_on_newline # Cases inside a switch should always be on a newline
+trailing_whitespace:


So this is giving you a warning when you add a whitespace at the end of a line, but skips empty lines?

elland

Thanks for taking the time to do such a major rewrite. Did a first pass read, left some comments about grammar/spelling and asked for clarification on some topics. 👍

elland · 2018-05-03T08:53:34Z

GIT_AND_GITHUB.md

- If it's a static UI, a screenshot would do. 
- If it's an interaction, a gif would help a lot. A good tool to make gifs is [Licecap](http://www.cockos.com/licecap/).
+- If it's a static UI, a screenshot will do. 
+- If it's an interaction, a `gif` would help a lot. Goods tool to make `gif`s include [Licecap](http://www.cockos.com/licecap/), [GIPHY Capture](https://itunes.apple.com/us/app/giphy-capture-the-gif-maker/id668208984?mt=12), and [GIF Brewery](http://gifbrewery.com/).


Goods tool, Good tools? 😅

🤦‍♀️

elland · 2018-05-03T08:56:27Z

GIT_AND_GITHUB.md


-After you have merged, you should delete the branch and smile. The smile is critical 
-part of the process, so don't forget this.
+After you have merged, you should delete the branch and smile. The smile is critical part of the process, so don't forget this.


…is _a_ critical part… 👍

elland · 2018-05-03T08:59:05Z

README.md

@@ -1,23 +1,26 @@
 ![logo](https://raw.githubusercontent.com/bakkenbaeck/iOS-playbook/master/assets/logo-v1.png)

-Don't forget to check out our favourite [suite of components and helper methods](http://github.com/usesweet), to avoid duplication of work reimplementing them yourself..
+Don't forget to check out our favorite [suite of components and helper methods](http://github.com/usesweet) and the [other B&B iOS repos](https://github.com/bakkenbaeck?language=swift) to see if we've already built something to help with what you're trying to do.


Overcorrected favourite here? 👀

I updated all the spelling to be en-US since I was doing most of the writing and editing and that's what I'm used to.

elland · 2018-05-03T09:00:09Z

REUSABLE_COMPONENTS.md

 - [ ] Create a local folder with your project name and run `swiftplate` from there
 - [ ] 👶  Author: Your own name
 - [ ] 📫  Author email: [email protected]
 - [ ] 🌍  GitHub URL: https://github.com/bakkenbaeck/<YOUR_PROJECT_NAME>
- [ ] 🏢  Organization Name: Bakken & Baeck
+- [ ] 🏢  Organization Name: Bakken & Bæck


elland · 2018-05-03T09:00:53Z

REUSABLE_COMPONENTS.md

-Before shipping your component make sure that the README states what makes your Pod different than the other ones, been made by you might not be enough. It's very important that your README is :star2: fabulous :star2:.
+- [ ] Make sure that the README states what makes this component different than the other ones - simply having been made by us is probably not enough. It's very important that your README is 🌟 fabulous 🌟.
+- [ ] Provide a super-simple example on how to get the component up and running. 
+- [ ] If the component is dependent on other frameworks, explain why they're needed them and what they do. Don't assume that people know everything.


why they're needed them that doesn't sound right 😅

no it does not

elland · 2018-05-03T09:16:00Z

STYLE_GUIDE.md

+
+Images should be named consistently within an app to preserve organization and developer sanity. It is strongly suggested to append the state name to images for non-default states.
+
+Images should be named in either `camelCase` or  `lower_snake_case` to facilitate code generation based on the image names. Pick one and stick to it per project. 


I'd add here something along the lines of avoid using hyphens as it messes up code completion.

elland · 2018-05-03T09:16:20Z

STYLE_GUIDE.md

+- `ic_refresh` and `ic_refresh_selected` for a refresh icon and its selected state used in multiple places throughout an app
+- `articleNavigationBar` and `articleNavigationBarSelected` for  something used in one specific place. 
+
+### Localized strings


Localised 😁

There are a bunch of those s/s/z/ changes. I'd really appreciate if we could keep the spelling consistent across documents (🇬🇧). 👍

I have tried to keep this consistent within the current repo. If there's something I missed, please let me know.

elland · 2018-05-03T09:18:47Z

STYLE_GUIDE.md

+
+Any strings visible to the user must be localized using `NSLocalizedString` or something wrapping it. It is way, way easier to set up all strings like this from the beginning than to add this feature later. 
+
+Don't use the developer-language content of the string as the localized string's key, because if you do, you then have to update the copy in multiple places. 


not sure I follow.

Basically if you have

NSLocalizedString("Something in English", "Title of some view controller")

Your Localizable.strings files will contain

Base.lproj:

/* Title of some view controller */ "Something in English" = "Something in English";

es.lproj

/* Title of some view controller */ "Something in English" = "Alguna cosa en español";

So if you wind up changing the key, you have to update not just the key at the point where you're wrapping it in NSLocalized string, but you also have to update it in the Localizable.strings in Base.lproj and es.lproj, times whatever number of languages you're supporting.

Otherwise, the localized string system will just return the updated key, so it'll return everywhere in English.

ok I get what you meant now 👍🏽

elland · 2018-05-03T09:19:22Z

STYLE_GUIDE.md

+
+Don't use the developer-language content of the string as the localized string's key, because if you do, you then have to update the copy in multiple places. 
+
+Use `lower_snake_case` for localized string keys to both facilitate code generation and to make it extremely obvious when there is an untranslated user-facing string, since by default if a value does not exist for a key in `Localizable.strings`, the key itself is returned.


There should be a way to know the key is missing when compiling. 😭

I wrote a test helper for this years ago https://github.com/designatednerd/DNSiOSLocalizationTestHelpers - it would certainly be nice to know at compile time though

elland · 2018-05-03T09:21:47Z

STYLE_GUIDE.md

+
+## Linting
+
+For code lint we use [SwiftLint](https://github.com/realm/SwiftLint). Our current `.swiftlint.yml` file is available [here](default.swiftlint.yml).


Does this point to the local file? I'm not sure how markdown links inside github work.

… Toshi

designatednerd · 2018-05-03T14:11:54Z

@elland Anything further you'd like me to address?

designatednerd · 2018-05-03T17:27:41Z

GIT_AND_GITHUB.md

-a fix? Well, if you find a bug in a previous release, you can just, checkout the tag,
-fork it, fix it, deploy the new build, and finally, submit a PR to merge it with `master`
-to the sound of Daft Punk.
+You might be wondering: why only `master`? What if I have to rollback to apply a fix? Well, if you find a bug in a previous release, you can just, checkout the tag, fork it, fix it, deploy the new build, and finally, submit a PR to merge it with `master` to the sound of Daft Punk.

 Commit messages are in the [imperative present tense](http://stackoverflow.com/questions/3580013/should-i-use-past-or-present-tense-in-git-commit-messages), and have no period at the end.


note to self for tomorrow: remove this line

elland · 2018-05-28T13:52:58Z

BEST_PRACTICES.md

+
+The physical files should be kept in sync with the Xcode project files in order to avoid file sprawl. Any Xcode groups created should be reflected by folders in the filesystem. Code should be grouped by type and feature for greater clarity.
+
+Make sure to use an [app-template](https://github.com/bakkenbaeck/app-template) for consistency.


Are we going to use this? I haven't in a long time.

I have not since I've been on the same project since I started. In the past I have found it extremely helpful to have an app template, especially for things like "Setting up a bunch of boilerplate related to CI"

We should evaluate whether the current template is up-to-date for what we need there.

That would make more sense. Actually, can't we build something that integrates with Xcode anymore? So we can start the template from the new project tab?

Yes, I fought with this at a prior job. However, I think that's more of an issue for that repo than for this one. I think we should still point to a common template here.

csdodd · 2018-06-05T06:30:07Z

Great work Ellen. I'm gonna merge as there is some consensus here. If there are still disagreements I suggest they be taken in a separate PR. This is a big change and I'd rather merge it in now and amend later rather than letting the PR get stale.

designatednerd added 11 commits April 24, 2018 23:52

Separate style guide from best practices and shorten up requirements

02b6c37

Move project structure into best practices instead of a separate doc

217c552

clean up formatting on git and github doc, minor condtent adjustments

a557e86

clean up reusable components, remove outdated references

125fd4b

clean up the general communication guide

e285ebd

Quick update of the releases page

b273b92

Cleanup hiring and combine repetitive bits.

9c429ee

move stuff in releases to git + github

50e209d

archive topic suggestions and weekly meetings,

012f440

Add default swiftlint file

e0158ee

Update readme.

8156375

designatednerd requested review from marijnschilling, elland, yuliaveres and roberthein April 25, 2018 13:43

elland reviewed Apr 25, 2018

View reviewed changes

fix grammars

35a05a8

onmyway133 reviewed Apr 26, 2018

View reviewed changes

marijnschilling reviewed Apr 26, 2018

View reviewed changes

designatednerd added 2 commits April 26, 2018 16:07

make sure title capitalization is consistent + everything has an accu…

8faab8a

…rate table of contents

Merge branch 'master' into robots + fix merge conflicts

01881ae

# Conflicts: # BEST_PRACTICES.md

roberthein approved these changes May 2, 2018

View reviewed changes

spel mor gud

a874c5d

elland reviewed May 3, 2018

View reviewed changes

marijnschilling approved these changes May 3, 2018

View reviewed changes

elland suggested changes May 3, 2018

View reviewed changes

goods tool != good tools

f93bc7f

designatednerd added 6 commits May 3, 2018 13:06

missing a

bb7e666

rm excess word

5824d7c

clarify why we're using certain casing conventions

1bd0059

Clarify reasoning for private unit test rule

e3a6e29

last remaining en_GB -> en_US

b1c334f

add a couple of exclusions to rules after trying to implement this on…

94e308b

… Toshi

designatednerd mentioned this pull request May 3, 2018

Add best practices for dealing with localisation #24

Open

designatednerd commented May 3, 2018

View reviewed changes

designatednerd added 2 commits May 4, 2018 17:50

kill off requirement about commit messages

769e177

Better explanations of what rules we're partially disabling and why

475823f

elland reviewed May 28, 2018

View reviewed changes

csdodd merged commit eb1add5 into master Jun 5, 2018


		- I see you are using an optional as an array of User object in the example above, have you considered to have that property default to an empty array?

		@@ -1,51 +1,55 @@
		# Communicating within the iOS team
		# Communicating With Your Colleagues

		@@ -1,51 +1,62 @@
		# Communicating within the iOS team
		# Communicating with your colleaugues


		In every other case though: 🚨! It's good practice to elaborate. It may seem inefficient to be so verbose in our replies, but it will avoid misunderstandings, which can result in far greater inefficiency.

		## Criticizing or asking questions


		Images should be named consistently within an app to preserve organization and developer sanity. It is strongly suggested to append the state name to images for non-default states.

		Images should be named in either `camelCase` or `lower_snake_case` to facilitate code generation based on the image names. Pick one and stick to it per project.


		Any strings visible to the user must be localized using `NSLocalizedString` or something wrapping it. It is way, way easier to set up all strings like this from the beginning than to add this feature later.

		Don't use the developer-language content of the string as the localized string's key, because if you do, you then have to update the copy in multiple places.


		Don't use the developer-language content of the string as the localized string's key, because if you do, you then have to update the copy in multiple places.

		Use `lower_snake_case` for localized string keys to both facilitate code generation and to make it extremely obvious when there is an untranslated user-facing string, since by default if a value does not exist for a key in `Localizable.strings`, the key itself is returned.


		## Linting

		For code lint we use [SwiftLint](https://github.com/realm/SwiftLint). Our current `.swiftlint.yml` file is available [here](default.swiftlint.yml).


		The physical files should be kept in sync with the Xcode project files in order to avoid file sprawl. Any Xcode groups created should be reflected by folders in the filesystem. Code should be grouped by type and feature for greater clarity.

		Make sure to use an [app-template](https://github.com/bakkenbaeck/app-template) for consistency.

Simplification, Clarification, Robotification #118

Simplification, Clarification, Robotification #118

Conversation

designatednerd commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberthein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marijnschilling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

designatednerd commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csdodd commented Jun 5, 2018