Backwards compatibility in 2.0 (and the breaking thereof) #167

Closed
jspahrsummers opened this Issue Oct 24, 2013 · 13 comments

Comments

Projects
None yet
4 participants
Owner

jspahrsummers commented Oct 24, 2013

We've discussed backwards compatibility in a couple of 2.0 issues, including #149 and #153.

My stance until now has been that we should preserve as much backwards compatibility as possible, and apply deprecated notices whenever we introduce new-and-improved APIs. However, I'm starting to rethink this, because it's resulting in a lot of weird and confusing renames that sometimes sound worse.

Therefore, I'd like to stop renaming things when it's only for migration, and figure out some other way to introduce project-wide breakage that forces users to consult a changelog/migration guide.

Overhauling a project to use a new major library version is no small task — so we still need to be really conscious of where and why we're breaking compatibility — but I'd rather make migration a little harder than have a bunch of duplicative and confusing APIs hanging around.

@robb

Owner

jspahrsummers commented Oct 24, 2013

Only a few ideas come to mind right now:

  • Make a highly-visible API, like MTLModel, as unavailable, and point users to a migration guide in the message. We'd have to figure out what the new API should look like, though.
  • Put an #error directive into a highly-visible header, like Mantle.h, and point them to a migration guide. We'd have to figure out which header, and what that header's replacement should be.
  • Require users to define a macro like I_UPGRADED_TO_MANTLE_2_0 (a la ReactiveCocoa/ReactiveCocoa#878), and mark changed APIs as unavailable when it's not defined. Although users would have to pollute their [prefix] header(s) with the macro, we could put more helpful messages on the actual APIs that broke (instead of one generic umbrella message).
Owner

robb commented Oct 24, 2013

I'm totally on board with this. Besides the namespace pollution, there's also the issue that some changes in 2.0 mean a complete u-turn: @{} now means no mappings where it meant map everything explicitly and the managed object adapter reversed the transformer direction. Maintaining backwards compatibility means increased code complexity.

Make a highly-visible API, like MTLModel, as unavailable

I could see this happen more-or-less naturally in #151. -[MTLJSONAdapter JSONDictionary] has already been replaced with a version that has an error parameter. If it doesn't happen naturally, at least we can confine all awkward renaming to the adapter classes, since they are where the semantic changes manifest anyway.
If a user doesn't use MTLJSONAdapter or MTLManagedObjectAdapter then the changes 2.0 shouldn't affect them.

Require users to define a macro like I_UPGRADED_TO_MANTLE_2_0

I think this is what I prefer, right now. When would we plan to remove this macro, though – 3.0 or sooner?

EDIT: That being said, I wonder if it's wise to decide on a method of project-wide breakage at this point. We can agree now to maximize backwards compatibility but minimize confusing renaming. Once the release candidate is near and we can realistically migrate a project, we will have a better understanding of the upgrade-path and then decide on this issue.

Contributor

mdiep commented Oct 24, 2013

Require users to define a macro like I_UPGRADED_TO_MANTLE_2_0

I think it's better to invert this like RAC: WE_PROMISE_TO_UPGRADE_TO_MANTLE_2_0. The default for new projects using Mantle should be the 2.0 behavior.

Owner

robb commented Oct 24, 2013

The default for new projects using Mantle should be the 2.0 behavior.

totes

robb referenced this issue Oct 25, 2013

Merged

Adding MTLTransformerErrorHandling #153

4 of 4 tasks complete
Owner

jspahrsummers commented Oct 25, 2013

The default for new projects using Mantle should be the 2.0 behavior.

This defeats much of the point, unfortunately. The point of such a macro would be to introduce deprecation warnings, so users know exactly what broke and how to fix it.

If the macro's meaning is flipped, then that helpfulness is lost. Who's going to opt-in to a macro that gives them compiler errors?

Owner

robb commented Oct 25, 2013

oh, right. Any idea how long that macro would stick around, tho?

FWIW, we should be able to put that #define statement in for people going straight to the 2.0 CocoaPod.

Owner

jspahrsummers commented Oct 28, 2013

Any idea how long that macro would stick around, tho?

Until 3.0, probably.

I know it's sucky. That's why it was in a list of sucky solutions. :\

Contributor

mdiep commented Oct 28, 2013

The default for new projects using Mantle should be the 2.0 behavior.

This defeats much of the point, unfortunately. The point of such a macro would be to introduce deprecation warnings, so users know exactly what broke and how to fix it.

I disagree. Now that I've read through more of the context, I'd suggest a different name (like SHOW_DEPRECATIONS_FROM_MANTLE_2_0), but I think the key piece is just to make that available. Users should be expected to read some documentation when upgrading to a major version. Putting information about this macro in whatever documentation they're most likely to read makes it accessible.

This is doubly true if there are any obvious breakages—compile errors or warnings.

If the macro's meaning is flipped, then that helpfulness is lost. Who's going to opt-in to a macro that gives them compiler errors?

Honestly? Anyone who's worth considering with this sort of decision.

Owner

robb commented Oct 28, 2013

If we assume that people will read the documentation, then there is no need to introduce breakage, right? :trollface:

That being said, I'm more concerned with improperly pinned Podfiles than I am with submodules, so I'd be fine with a SHOW_DEPRECATIONS_FROM_MANTLE_2_0 macro assuming it can be injected once people cross the 2.0 line

EDIT: added trolleybus

Owner

jspahrsummers commented Oct 28, 2013

… Users should be expected to read some documentation when upgrading to a major version. …

… If we assume that people will read the documentation …

lulz

Honestly? Anyone who's worth considering with this sort of decision.

My point was: if they would opt-in to see deprecation errors, what's the point of even putting those in the code? They might as well go into a migration guide that's required reading.

I like the 2nd option. Switching to using Mantle2.h as the main header would allow you to have an error in Mantle.h that says "If this is a new project, use Mantle2.h. If you're migrating from Mantle 1.x, read the migration document: http://.....", which seems like it would be a lot less annoying for new users than requiring them to define a macro.

Owner

jspahrsummers commented Mar 13, 2014

This may be a moot discussion now. I think we've introduced some changes that will actually break compilation. I'm certainly not opposed to doing so, in any case—we have a lot of good things coming down the pipeline that I don't really want to include shims for.

Owner

robb commented Mar 13, 2014

Once we have a RC or at least a feature freeze, we can try migrating e.g. OctoKit to see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment