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

ᴡᴀᴛᴄʜ #34

Merged
merged 10 commits into from
Jun 24, 2015
Merged

ᴡᴀᴛᴄʜ #34

merged 10 commits into from
Jun 24, 2015

Conversation

esttorhe
Copy link
Member

Fix #33

Adds support for ᴡᴀᴛᴄʜ framework.

Dependencies

This PR depends on #32 (because its using the «new» throws/guard inits on the XcodeServerConfig class

@esttorhe esttorhe changed the title Watch os ᴡᴀᴛᴄʜ Jun 24, 2015
@@ -197,6 +239,7 @@
children = (
3A7B48BC1B2A5AC40077ABEA /* XcodeServerSDK */,
3A7B48C91B2A5AC40077ABEA /* XcodeServerSDKTests */,
11376BD11B3A2F910005A681 /* XcodeServerSDK - ᴡᴀᴛᴄʜ */,
Copy link
Member

Choose a reason for hiding this comment

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

Why this new folder reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

This folder holds the public header for the ᴡᴀᴛᴄʜ framework (its different than the other ones; unless we provide target conditionals

@czechboy0
Copy link
Member

Could you possibly take out the changes from #32 here so that we can merge just the watchOS support? I think #32 will require more work before we can merge it (and keep the project buildable).

@czechboy0
Copy link
Member

Also, please add some support section to the readme so that we can properly declare the support of all platforms (iOS/OS X/watchOS)!

@esttorhe
Copy link
Member Author

#32 doesn't stop the project from being buildable; I even provided a personal project where I'm using Carthage with iOS and watchOS support from my own branch (that points to the one backing this PR) and the project is building and generating the correct frameworks as expected.

Regarding the README I'll try to find some time for that later today.

@czechboy0
Copy link
Member

Can you merge your swift-2 branch into this branch? I think there are some changes that we already merged... Also, are you sure having the Apple symbol in the folder/file names going to be safe? 😄

@esttorhe
Copy link
Member Author

@czechboy0 done; just rebased from remote swift-2 instead of my local swift-2.

Regarding the name; its just unicode; don't think it will choke at any point unless  decides to regress and stop supporting them; i'll make the change if needed though

@czechboy0
Copy link
Member

Ok, can you just do a final check that if you try to run all three platform's schemes one by one and test them, you get the same results? (Number of tests, all succeeding). Then it'll be ready for a merge.

@esttorhe
Copy link
Member Author

All test match on both infrastructures (ᴡᴀᴛᴄʜ doesn't support tests yet).

Actually OS X and iOS use the same testing bundle and clases except as separate targets to «compile» against the different infrastructure.

PS. I added the supports part to the README; I think that perhaps having a CHANGELOG.md would be good to keep track of everything that changes in between releases and also to better keep track of contributions, etc (not that I'm trying to still your ⚡ , just that the README mostly reflects this as a solo project from you and might deter others from contributing, just my 2cents)

@czechboy0
Copy link
Member

@esttorhe Sounds good, feel free to add a changelog.md! Me not having it is not in an attempt to protect my ⚡ ⚡, I'm just new to managing an open source project. All ideas are welcome (especially good ones).

@esttorhe
Copy link
Member Author

My comment wasn't aim as an attack but rather to show that I wasn't trying to steal the idea 🙇

I'm about to commit the change to platforms instead; which its a far better word.

I'll create the CHANGELOG.md as a separate PR and perhaps a good way to proceed from now on is that every PR should come with a new line in the CHANGELOG as well; that way its easily trackable; kind of like Artsy does with their projects (Eigen for example)

Better reflect what they are.
@czechboy0
Copy link
Member

Haha I know it wasn't.

Sounds good, let's start doing that. Maybe also draft a contributing.md doc that explains how to do this? (Or if it's too much, we can split it into a separate PR). 👍

@czechboy0
Copy link
Member

Maybe also add minimum deployment targets for each platform? Those can be found in our Podfile.

@esttorhe
Copy link
Member Author

Hopefully 080d47e is the one 😂

@czechboy0
Copy link
Member

Let me know when you add the contributing list so that I can merge it 😉

@esttorhe
Copy link
Member Author

Feel free to merge now if you are ok with the PR.

I was planning on taking the time to go over all the closed PRs and trying to organize them in the file (although I might end up just doing the list of PRs with the author next and we won't «tag» each release until next one since the CHANGELOG was conceived until now).

@czechboy0
Copy link
Member

Yeah, this one might be good inspiration.

@czechboy0
Copy link
Member

They have a master area at the top, which just gets cut off and converted into a release whenever one happens. Something we can easily adopt as well.

@esttorhe
Copy link
Member Author

That looks promising. Always liked the CP change log as well.

Perhaps explore using this https://github.com/skywinder/github-changelog-generator + the master area?

What do you think?

@czechboy0
Copy link
Member

This tool is pretty awesome. I'd love to not have to ask people to add manual steps when sending PRs. Let's start using this and see how it goes. I'll commit the first changelog after we merge this.

czechboy0 pushed a commit that referenced this pull request Jun 24, 2015
@czechboy0 czechboy0 merged commit 73437a4 into buildasaurs:swift-2 Jun 24, 2015
@czechboy0 czechboy0 mentioned this pull request Jun 24, 2015
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.

2 participants