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 to Ruby v3.0 #3265

Closed
wants to merge 11 commits into from
Closed

Upgrade to Ruby v3.0 #3265

wants to merge 11 commits into from

Conversation

briri
Copy link
Contributor

@briri briri commented Dec 13, 2022

Fixes #3225

  • Upgrade to Ruby version 3.0.5
  • Bumped all Github actions to use ruby 3.0
  • Cleaned up Gemfile by:
    • removing gems that were already commented out
    • removed selenium-webdriver and capybara-webmock
    • removing version restrictions on: danger, font-awesome-sass, webdrivers
  • Ran into issue with @import 'font-awesome-sprockets'; line in app/assets/stylesheets/application.scss. Removed that line after referring to the latest font-awesome install/setup guide which no longer includes it.
  • Updated places that were incorrectly using keyword args. See this article for an overview
  • Removed .freeze from Regex and Range constants since those types are already immutable
  • Fixed Rubocop complaint about redundancy of r.nil? ? nil : r.user, so changed it to r&.user in app/models/plan.rb
  • Fixed Rubocop complaint about redundant :: in config.log_formatter = ::Logger::Formatter.new in config/environments/production.rb
  • Froze deprecator constants that were Strings
  • Cleaned up spec/rails_helper.rb and spec/spec_helper.rb
  • Simplified the spec/support/capybara.rb helper to work with the latest version of Capybara and use its built in headless Chrome driver

@briri
Copy link
Contributor Author

briri commented Dec 13, 2022

@pengyin-shan I updated the Changelog but am not sure if I did it correctly though. I think the error above is with the Dangerfile itself

@briri
Copy link
Contributor Author

briri commented Dec 13, 2022

I still need to work on the tests. I have them running locally, but GitHub actions don't seem to be happy.

@pengyin-shan
Copy link
Contributor

@pengyin-shan I updated the Changelog but am not sure if I did it correctly though. I think the error above is with the Dangerfile itself

Let me merge my previously fixed Danger PR...

@pengyin-shan
Copy link
Contributor

I still need to work on the tests. I have them running locally, but GitHub actions don't seem to be happy.

Is your local Danger not happy? It seems the Danger Github action is working fine

@pengyin-shan
Copy link
Contributor

@briri for test related settings, this is currently what we have in Dangerfile:

has_test_changes = !git.modified_files.grep(/spec/).empty?

if git.lines_of_code > 50 && has_app_changes && !has_test_changes
  warn('There are code changes, but no corresponding tests. ' \
       'Please include tests if this PR introduces any modifications in ' \
       'behavior. \n
       Ignore this warning if the PR ONLY contains translation.io synced updates.',
       sticky: false)
end

Do you think we need to change/update anything?

@briri
Copy link
Contributor Author

briri commented Dec 13, 2022

Is your local Danger not happy? It seems the Danger Github action is working fine

My local Danger seems ok. The GitHub tests report an issue starting up Rails. They run fine in

@briri briri closed this Dec 13, 2022
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.

2 participants