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 the Appraisal Gemfiles. #784

Merged
merged 3 commits into from
Mar 17, 2017
Merged

Update the Appraisal Gemfiles. #784

merged 3 commits into from
Mar 17, 2017

Conversation

nickcharlton
Copy link
Member

@nickcharlton nickcharlton commented Mar 17, 2017

  • Reduce the bourbon requirement slightly (7 to 6) so we can avoid updating Sass.
  • Adds refills, autoprefixer-rails, sentry-raven which were previously merged.

Appraisals Outdated
@@ -1,5 +1,5 @@
appraise "sass-3-4" do
gem "sass", "~> 3.4"
gem "sass", "~> 3.4.22"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this level of specificity cause issues for projects that have versions 3.4.0..3.4.21?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh, I hadn't thought of that. What do you think would be a better way of specifying it?

Copy link
Collaborator

@jayroh jayroh Mar 17, 2017

Choose a reason for hiding this comment

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

Depends, I think. What does administrate need from .22 that .0 doesn't provide? Looks like it's Bourbon? Can the bourbon version be bumped backwards a little bit to accommodate some flexibility in that realm.

If possible, I'd suggest changing the Administrate sass/bourbon requirements to NOT use any features that are locked into later/younger versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the bourbon version be bumped backwards a little bit to accommodate some flexibility in that realm.

Yes, we can and probably should do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've switched the Bourbon requirement to be 5.0.0.beta.6 or greater and this seems to solve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I opened thoughtbot/bourbon#1015 on Bourbon to loosen these requirements.

* Bourbon requires sass 3.4.22, which is minor bump.
* Adds refills, autoprefixer-rails, sentry-raven which were previously merged.
@nickcharlton nickcharlton force-pushed the nc-update-appraisals branch from 38150be to 89241f6 Compare March 17, 2017 19:42
gem "unicorn"
gem "rails", "~> 4.2.0"

group :development do
gem "refills"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest removing Refills because it is going through a major rewrite. The gem is likely to go away (or dormant).

The gem is just a generator anyway, so we don’t need to have it to use some of the components.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, cool! I'll do that in the same PR and merge. Thank you!

From @tysongach:

> I suggest removing Refills because it is going through a major rewrite.
> The gem is likely to go away (or dormant).
> The gem is just a generator anyway, so we don’t need to have it to use
> some of the components.
@nickcharlton nickcharlton merged commit 182ad50 into master Mar 17, 2017
@nickcharlton nickcharlton deleted the nc-update-appraisals branch March 17, 2017 21:02
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