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

Swift 4 Compatibility - No NSURLConnection Refactor #21

Merged
merged 12 commits into from
Jan 30, 2018

Conversation

krze
Copy link

@krze krze commented Jan 19, 2018

I did a little grunt work here converting the project to Swift 4 from Swift 2. There's still a warning regarding the deprecation of NSURLConnection. The CCBufferedImageView needs refactoring to be up to date with URLSession, but that's going to be a bigger effort.

I figured the first step would be to make this repo easily compatible with Xcode 9 😛

The tests all pass, I regenerated the reference images for the snapshot cases and they match the spec.

@krze krze changed the title Swift 4 Compatibility - No Refactor Swift 4 Compatibility - No NSURLConnection Refactor Jan 19, 2018
@loudmouth
Copy link
Contributor

Hey @krze thanks so much for doing all this work! I'll review the changes today!

As you may have gathered by looking at the other Github issues, this repo is no longer receiving full support from Contentful, but if the maintenance burden is low maybe we can figure out a way to keep the repo building and working.

Out of curiosity, are you using Concorde for a personal project or for more professional work?

@loudmouth loudmouth self-requested a review January 24, 2018 12:09
@loudmouth loudmouth self-assigned this Jan 24, 2018
@loudmouth loudmouth removed their request for review January 24, 2018 12:10
@krze
Copy link
Author

krze commented Jan 24, 2018

In this case it was professional, I was investigating progressive JPG solutions for our existing image stack and Concorde looked like a good option. It's not in production but we were pleased about how easily it integrated. We were actually kind of shocked how readily it worked because our stack is a bit ancient.

Still really cool stuff, libjpeg is interesting to work with

@loudmouth
Copy link
Contributor

cool! It may be that I get to this review tomorrow rather than today as other priorities are more pressing.

The reason I asked about your use case is that I'm trying to figure out the future of this project. You may have noticed that's under our -labs GitHub org which means it is officially an "experimental unofficial" project by our organization's standards and needs. Depending on your interest level, maybe we can get in contact outside of GitHub and ponder the future of Concorde together...

@loudmouth
Copy link
Contributor

Hey @krze, the PR looks great, but I don't want to merge until the tests are passing again. Unfortunately, I don't have time at the moment to work on passing the tests, but if you are willing ot keep going and get everything passing again, I'll be happy to merge this PR.

Once again, thanks so much for all the hard work 🙏

Copy link
Contributor

@loudmouth loudmouth left a comment

Choose a reason for hiding this comment

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

You might consider adding nullability specifiers to relevant Objective-C classes like CCBufferedImageDecoder so that we don't always get optionals in Swift-land after initializing an instance of the decoder.

@krze
Copy link
Author

krze commented Jan 29, 2018

@loudmouth Added the nullability checks and got the tests to run. The Snapshot tests are failing in CI but passing locally, despite using the same simulator in both environments. I'm not sure why they're being picky in CI - running make setup then make test will result in 100% pass locally

@krze
Copy link
Author

krze commented Jan 29, 2018

Scratch that: added a env var check in the tests to generate snapshots in CI. The tests will now clean and generate snapshots before running the test for real. Exit status is determined from the second test run

@loudmouth
Copy link
Contributor

Wow. Thanks so much @krze for taking this across the finish line!

@loudmouth loudmouth merged commit 0ad7205 into contentful-labs:master Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants