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

Upgrade most of gems #742

Merged
merged 2 commits into from Jun 25, 2020
Merged

Upgrade most of gems #742

merged 2 commits into from Jun 25, 2020

Conversation

JuanitoFatas
Copy link
Member

@JuanitoFatas JuanitoFatas commented Jun 4, 2020

This PR:

  • Use Rails 6 and other gems
  • Lock rouge because syntax highlight css classes changed (we should upgrade this separately)
  • Simplify Page#render

@harrietgrace harrietgrace temporarily deployed to docs-review-upgrade-gem-jacc3l June 4, 2020 13:52 Inactive
@yob
Copy link
Contributor

yob commented Jun 4, 2020

Upgrade all of the things!

My personal preference is to upgrade gems in smaller batches over multiple PRs, so testing each PR is easier and any surprises are easier to track down.

This is a pretty simple app though, so maybe we can get away with all-at-once?

@JuanitoFatas
Copy link
Member Author

My personal preference is to upgrade gems in smaller batches over multiple PRs

We can turn on dependantbot and configure it in a less noisy way.

This is a pretty simple app though, so maybe we can get away with all-at-once?

The tests passed and clicking through the reviews app everything looks okay.

@plaindocs
Copy link
Contributor

plaindocs commented Jun 15, 2020

I'm 👍 on dependabot ongoing.

I've clicked around the review app and looks fine.

Thanks for diving in @JuanitoFatas .

@JuanitoFatas JuanitoFatas marked this pull request as ready for review June 15, 2020 13:31
@harrietgrace harrietgrace temporarily deployed to docs-review-upgrade-gem-jacc3l June 15, 2020 13:33 Inactive
av.render(*args)
ActionController::Base.append_view_path "app/views/pages"
renderer = ActionController::Base.renderer.new
renderer.render(partial: args.first)
Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewd What do you think about the change here? 🙇‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

The first line is mutating a global value, but this method gets called multiple times.

Sounds to me like the append_view_path should be in PagesController, then this would be:

def render(partial)
  PagesController.render(partial: partial)
end

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Fixed in 4488f93.

* Rails 6
* Lock rouge because syntax highlight changed
* Use ActionController::Base renderer
@harrietgrace harrietgrace temporarily deployed to docs-review-upgrade-gem-jacc3l June 24, 2020 04:32 Inactive
@JuanitoFatas JuanitoFatas reopened this Jun 24, 2020
@harrietgrace harrietgrace temporarily deployed to docs-review-upgrade-gem-ctpaqa June 24, 2020 04:34 Inactive
@JuanitoFatas JuanitoFatas reopened this Jun 24, 2020
@harrietgrace harrietgrace temporarily deployed to docs-review-upgrade-gem-ocopco June 24, 2020 07:25 Inactive
Co-Authored-By: Matthew Draper <matthew@trebex.net>
@harrietgrace harrietgrace temporarily deployed to docs-review-upgrade-gem-ocopco June 24, 2020 07:31 Inactive
@JuanitoFatas JuanitoFatas merged commit e749541 into master Jun 25, 2020
@JuanitoFatas JuanitoFatas deleted the upgrade-gems branch June 25, 2020 22:36
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

6 participants