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

Fix package dependency syntax #1105

Conversation

alexandrethsilva
Copy link

@alexandrethsilva alexandrethsilva commented Mar 30, 2020

@designatednerd I went ahead and made the PR related to #1102 in case you'd like to have it merged as I will probably be offline for a few hours.

One thing I'd like to ask though, is whether it would make sense to have the officially suggested syntax pointing to an exact version rather then a range and updating the docs as new versions are released, as in:

.package(name: "Apollo", url: "https://github.com/apollographql/apollo-ios.git", .exact("0.24.0"))

I ask for a few reasons:

  • You point out right below that the version should be an exact match to the one in the main project, but the unaware user may end up mixing things? That would also make it more in line with the iOS SDK setup docs which favour the version to be up to the next minor only?
  • It makes it easier to debug opened issues, since you'd know the exact version installed instead of having to guess or depend on the reporter double-checking it?
  • It avoids propagating new bugs to people that setup their projects earlier and end up pulling a later version in the range (even if minor) which contains a bug?

Let me know if any of this makes sense and whether I should update the PR to make it align with these points.

Cheers!

@apollo-cla
Copy link

@alexandrethsilva: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@designatednerd
Copy link
Contributor

I think this issue points to me needing to update something in Package.swift- you absolutely shouldn't need to do this manually.

Good call on the from though - I should probably update that to get up to the next minor to match our recommendation for the main repo

@alexandrethsilva
Copy link
Author

@designatednerd should I drop this PR for the time being or would you like me to maybe take a different approach?

@designatednerd
Copy link
Contributor

Let's leave this up for now, I'm working on a fix but until it's actually ready this will at least help anyone who's stuck

@designatednerd
Copy link
Contributor

@alexandrethsilva I'd like to close this out in favor of #1106 - I really appreciate you taking the time to make this but I want to have the extra context I've added in that PR. is that cool?

@designatednerd
Copy link
Contributor

Went ahead and merged #1106 so I'm gonna close this out - thanks again for helping figure this out!

@alexandrethsilva
Copy link
Author

Hi @designatednerd, sure! Glad that I could help out a bit. 🤓

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.

3 participants