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

Integration with appraisal #4

Merged
merged 7 commits into from
Aug 20, 2013
Merged

Integration with appraisal #4

merged 7 commits into from
Aug 20, 2013

Conversation

jtmalinowski
Copy link
Collaborator

Hey,
So I took a look at it. I think main source of the problem was that you generated rails 4.0 app, it would probably be easier to upgrade generated 3.1 or 3.2 but I downgraded dummy to be compatible with 3.1 - 4.0

[Edit] You will probably have some suggestions, even though it is an early version. Integration with Appraisal should, however, not really change much, if at all. It is mostly implemented similarly to many other repos.

@zpao
Copy link
Member

zpao commented Aug 11, 2013

Awesome! Comments incoming inline

@@ -0,0 +1,11 @@
appraise "rails31" do
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to be unnecessarily picky and ask for better naming of these. rails-3.1, rails-3.2, rails-4.0 (unless that breaks things but I don't think it did)

There is no reason to keep them. Just as we don't keep Gemfile.lock for a gem,
there is no point in enforcing particular version of gems when testing
different versions of rails with appraisal.
@jtmalinowski
Copy link
Collaborator Author

Well, if you consider this article by Yehuda I don't see any reason to keep gemfile locks for different rails versions. We should be compatible with what we define as dependencies, not with particular versions of gem. For now it works fine, but I'll ask someone about it.

@zpao
Copy link
Member

zpao commented Aug 11, 2013

Thoughtbot recommends we keep the .lock files checked in too (https://github.com/thoughtbot/appraisal/blob/master/README.md#version-control) so let's do that. I was just wishing that file didn't include specific user info in there. I don't think that removing that part of the file is actually a good idea, I don't know what side effects it would have.

@jtmalinowski
Copy link
Collaborator Author

OK, I'll keep an eye on this: thoughtbot/appraisal#49

@zpao
Copy link
Member

zpao commented Aug 20, 2013

Sorry for letting this linger! I'm going to merge in and then if there are any followups we can deal with them then.

Thanks for working on this!

zpao added a commit that referenced this pull request Aug 20, 2013
Integration with appraisal.

Closes #1
@zpao zpao merged commit c27d9ca into reactjs:master Aug 20, 2013
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.

None yet

2 participants