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

[Update] Add "MagicalRecord/Core" and "MagicalRecord/Shorthand" subspecs #7699

Merged
merged 2 commits into from
Feb 6, 2014

Conversation

siuying
Copy link
Contributor

@siuying siuying commented Feb 6, 2014

User needed MR_SHORTHAND enabled should use "MagicalRecord/Shorthand" instead.

Discussion: 9e7d7aa#commitcomment-5290889

…ecs.

User needed MR_SHORTHAND enabled should use "MagicalRecord/Shorthand" instead.

Discussion: CocoaPods@9e7d7aa#commitcomment-5290889
siuying referenced this pull request Feb 6, 2014
…Also

remove additions to prefix header — importing the header this way is not what
the MagicalRecord team recommends, and MR_SHORTHAND shouldn't be enabled by
default.
@@ -8,7 +8,8 @@ Pod::Spec.new do |s|
s.source = { :git => 'https://github.com/magicalpanda/MagicalRecord.git', :tag => "#{s.version}" }
s.description = 'Handy fetching, threading and data import helpers to make Core Data a little easier to use.'
s.source_files = 'Source/**/*.{h,m}'
s.framework = 'CoreData'
s.framework = 'CoreData'
s.requires_arc = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Did 1.8.3 require ARC? It's been a long time since I looked at that release…

@keith
Copy link
Member

keith commented Feb 6, 2014

@tonyarnold Let me know when this is ready.

@tonyarnold
Copy link
Contributor

1.8.3 was the last release that worked without ARC. @siuying can you remove the ARC line you added to this spec, and then it'll be right to go.

@siuying
Copy link
Contributor Author

siuying commented Feb 6, 2014

Sorry I missed that, will fix it

Set require arc to false.
@siuying
Copy link
Contributor Author

siuying commented Feb 6, 2014

@tonyarnold @Keithbsmiley fixed.

keith added a commit that referenced this pull request Feb 6, 2014
[Update] Add "MagicalRecord/Core" and "MagicalRecord/Shorthand" subspecs
@keith keith merged commit 40c085e into CocoaPods:master Feb 6, 2014
keith referenced this pull request Feb 6, 2014
I'll fix this properly in the next point release of MR.
@keith
Copy link
Member

keith commented Feb 6, 2014

Does it matter that the normal define is

#define MR_SHORTHAND

(Without the 0) via https://github.com/magicalpanda/MagicalRecord/blob/master/Docs/Installation.md

@rconnelly
Copy link

Was there a reason for modifying all the previous spec versions? This change broke our builds. Should you not have created a new spec with version 2.3?

@keith
Copy link
Member

keith commented Feb 7, 2014

Use pod 'MagicalRecord/Shorthand' for the old functionality.

@rconnelly
Copy link

Right we figured it out. Why did you guys switch the behavior of previous versions of specs? Should that have been a new version?

@tonyarnold
Copy link
Contributor

@Keithbsmiley the way that @siuying has defined it is fine — we're doing an #ifdef which only checks for the existence of the define, not for any specific value.

@keith
Copy link
Member

keith commented Feb 7, 2014

Defining the shorthand constant never should have been the default for these specs. We made the decision to change that. You can use the old style with the Shorthand subspec. This will remain the default for the future.

@NikoRoberts
Copy link

So to be clear, the decision was made to break compatibility for all code, for all versions, that used what someone considered to be the wrong method for defining shorthand.

You say it should be the default for the future but you just attempted to change the past as well...
We had a perfectly working locked version 2.2 and then it changed and broke everywhere

I know I have no right to be demanding about the work you guys do, but in my opinion this is bad versioning practice.

@tonyarnold
Copy link
Contributor

@NikoRoberts I've apologised in a bunch of different spots for the impacts this change had on people's projects, and thanks to @siuying there is a simple workaround for people that want to continue using the shorthand method syntax.

I'm not sure what other outcome you'd like to see?

@NikoRoberts
Copy link

@tonyarnold thank you for the apology and workaround but it still surprises me that the change wasn't reverted.

I understand wanting to change the behavior for future versions but existing versions that break when running pod install and this just seems like a mistake which is then "resolved" by all the users of shorthand have to change their code.
Does that sound right?

The fact that All automated deployments would break event though the podfile has locked version to me indicates that this commit should be reverted.

Am I incorrect in understanding all versions of the podspec were modified?
Or am I exaggerating the depth of affect this commit has?

@siuying
Copy link
Contributor Author

siuying commented Feb 20, 2014

@NikoRoberts The old version of the project has not been changed. What change is the build setting of the project, specified in the podspec. The setting (MR_SHORTHAND) is not the default setting of the library, and thus it should not the default setting of the podspec.

As a side note, if you want all automated deployment of old version still work even if the pod repo change, you really should check in the Pods folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants