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

Deprecate tasks in core/lib/tasks #2080

Merged
merged 4 commits into from
Jul 21, 2017
Merged

Conversation

cbrunsdon
Copy link
Contributor

@cbrunsdon cbrunsdon commented Jul 12, 2017

None of these tasks should be used by people.

This deprecates those tasks, and removes a dependency that exists just to support these tasks (highline).

@@ -1,10 +1,16 @@
require 'active_record'

def prompt_for_agree prompt
print prompt
return ["y", "yes"].include? STDIN.gets.strip.downcase

Choose a reason for hiding this comment

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

Redundant return detected.
Unnecessary spacing detected.

@@ -1,10 +1,16 @@
require 'active_record'

def prompt_for_agree prompt

Choose a reason for hiding this comment

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

Use def with parentheses when there are parameters.

@cbrunsdon cbrunsdon force-pushed the remove_highline branch 4 times, most recently from 9866727 to 605d576 Compare July 12, 2017 01:10
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I never used these tasks. Just left a small comment, it works for me!

core/db/seeds.rb Outdated
Rake::Task['db:load_dir'].reenable
Rake::Task['db:load_dir'].invoke(default_path)
['stores', 'store_credit', 'countries', 'return_reasons', 'states',
'stock_locations', 'zones', 'refund_reasons', 'roles', 'shipping_categories'].each do |seed|
Copy link
Member

Choose a reason for hiding this comment

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

I like formatting this kind of array with each element in a separate line, mainly to keep git commits history clean if we change something, but it's just my personal preference, I'm ok with this as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I also would like to have this separated by line 👍

And maybe use %w() syntax, but I don't care about this very much.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Great improvement.

core/db/seeds.rb Outdated
Rake::Task['db:load_dir'].reenable
Rake::Task['db:load_dir'].invoke(default_path)
['stores', 'store_credit', 'countries', 'return_reasons', 'states',
'stock_locations', 'zones', 'refund_reasons', 'roles', 'shipping_categories'].each do |seed|
Copy link
Member

Choose a reason for hiding this comment

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

Yes. I also would like to have this separated by line 👍

And maybe use %w() syntax, but I don't care about this very much.

namespace :db do
desc 'Loads a specified fixture file:
use rake db:load_file[/absolute/path/to/sample/filename.rb]'

task :load_file, [:file, :dir] => :environment do |_t, args|
ActiveSupport::Deprecation.warn("load_file has been deprecated. Please load your own file.")
Copy link
Member

Choose a reason for hiding this comment

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

Spree::Deprecation.warn

Copy link
Member

Choose a reason for hiding this comment

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

I think we can be a little more verbose/descriptive on this deprecation notice

rake spree:load_dir has been deprecated and will be removed with Solidus 3.0. Please load your seed files directly from your app's db/seeds.rb file.

@@ -13,6 +19,7 @@ use rake db:load_file[/absolute/path/to/sample/filename.rb]'

desc "Loads fixtures from the the dir you specify using rake db:load_dir[loadfrom]"
task :load_dir, [:dir] => :environment do |_t, args|
ActiveSupport::Deprecation.warn("load_dir has been deprecated. Please load your own files.")
Copy link
Member

Choose a reason for hiding this comment

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

Spree::Deprecation.warn

@@ -32,9 +39,9 @@ use rake db:load_file[/absolute/path/to/sample/filename.rb]'

desc "Migrate schema to version 0 and back up again. WARNING: Destroys all data in tables!!"
task remigrate: :environment do
require 'highline/import'
ActiveSupport::Deprecation.warn("remigrate has been deprecated. Please demigrate and then migrate again, I guess?")
Copy link
Member

Choose a reason for hiding this comment

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

I think rake db:reset is what we want to recommend, no?

Also Spree::Deprecation.warn

Nothing in this file uses highline, say_status comes from thor
Using load_dir doesn't provide us much benefit over calling all of them
ourselves here.
We're only using highline for a few, should-probably-not-be-used
methods rake tasks.
@cbrunsdon
Copy link
Contributor Author

Cleaned up the list of seeds and removed any trace of humour from the deprecations

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Nice

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

There's a word too much still in a deprecation notice, otherwise great!

@@ -13,6 +19,7 @@ use rake db:load_file[/absolute/path/to/sample/filename.rb]'

desc "Loads fixtures from the the dir you specify using rake db:load_dir[loadfrom]"
task :load_dir, [:dir] => :environment do |_t, args|
Spree::Deprecation.warn("rake spree:load_dir has been deprecated and will be removed with Solidus 3.0. Please load your files directly file.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please load your files directly file. I think there's a file too much.

Nothing here should be used by people.
@tvdeyen tvdeyen merged commit 4bd670a into solidusio:master Jul 21, 2017
DanielePalombo added a commit to nebulab/solidus that referenced this pull request Nov 27, 2020
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Dec 9, 2020
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Dec 10, 2020
DanielePalombo added a commit to nebulab/solidus that referenced this pull request Dec 11, 2020
DanielePalombo added a commit to nebulab/solidus that referenced this pull request Dec 11, 2020
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Dec 15, 2020
DanielePalombo added a commit to nebulab/solidus that referenced this pull request Dec 18, 2020
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Dec 21, 2020
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Jan 19, 2021
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Jan 28, 2021
kennyadsl pushed a commit to nebulab/solidus that referenced this pull request Jan 28, 2021
rmparr pushed a commit to rmparr/solidus that referenced this pull request Jun 1, 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.

5 participants