Add CLI - Integrate fakerbot 🤖 #1507

akabiru · 2019-01-05T00:18:23Z

What does this PR do?

Integrates the fakerbot gem into Faker as it's CLI

How should this be manually tested?

$ rake build
$ gem install pkg/faker-1.9.3.gem
$ faker

What are the relevant GitHub issues?

Closes #1505

Questions:

This seems like a major change to me, how are releases handled? Should we bump the version as part of this PR?

Screenshots

stympy · 2019-01-05T00:59:11Z

We won’t bump the version as part of this PR. The version bump will be separate.

akabiru · 2019-01-05T12:19:07Z

Sounds good! 👍🏾

The module dir `faker` houses the main "faker" namespaces; and it should remain that way. Best to have the CLI live outside the directory; similar to helpers

- Add integration test to cover that

akabiru · 2019-01-13T21:32:34Z

Hi @vbrazo , had a chance to review this PR? 🙂

The Reflector was doing too much; handling Search and List reflections which made it a tad harder to follow. This commit defines a Reflection interface and the correspoding subclasses.

akabiru · 2019-01-22T07:24:17Z

Hi @stympy @vbrazo , I recently made some improvements to the fakerbot reflection object that I thought would be useful here.

Looking forward to your review, please shout if there's anything I can do to make it smoother for you. Cheers. 🍻

vbrazo · 2019-01-24T15:44:27Z

Apologies for the slowness. This week has been busy. I started reading the files, but I still need more time to pull the code and check the work. I'll do that over the next days.

akabiru · 2019-01-24T17:44:15Z

No worries Vitor, totally understand 💯 . Please shout if you have any questions. 🙂

There's an erroneous 'null byte' error in the CI which could be caused by the bundler version. Further, As of Ruby 2.6.0, bundler is now built into Ruby, so no need to install it ourselves

vbrazo

Tested locally and liked what I saw

vbrazo · 2019-02-09T12:21:10Z

test/test_determinism.rb

@@ -45,7 +45,7 @@ def all_methods

  def subclasses
    Faker.constants.delete_if do |subclass|
-      %i[Base Bank Books Cat Char Base58 ChileRut Config Creature Date Dog DragonBall Dota ElderScrolls Fallout Games GamesHalfLife HeroesOfTheStorm Internet JapaneseMedia LeagueOfLegends Movies Myst Overwatch OnePiece Pokemon SwordArtOnline TvShows Time VERSION Witcher WorldOfWarcraft Zelda].include?(subclass)
+      %i[Base Bank Books Cat Char Base58 ChileRut CLI Config Creature Date Dog DragonBall Dota ElderScrolls Fallout Games GamesHalfLife HeroesOfTheStorm Internet JapaneseMedia LeagueOfLegends Movies Myst Overwatch OnePiece Pokemon SwordArtOnline TvShows Time VERSION Witcher WorldOfWarcraft Zelda].include?(subclass)


We should get rid of this big array and think about a better way to check this.

I agree, do we want to mark that as a separate piece of work?

Maybe we should raise that as a separate issue? At present, I'm afraid I don't think I have enough background to raise the issue for this case. 🙂

vbrazo · 2019-02-16T23:13:25Z

README.md

@@ -62,6 +62,14 @@ Faker::Name.name      #=> "Christophe Bartell"
 Faker::Internet.email #=> "kirsten.greenholt@corkeryfisher.info"
 ```

+### CLI


This change should go in our unreleased_README.md.

Good shout. 👍

vbrazo · 2019-02-16T23:15:50Z

lib/cli.rb

+    class Base < Thor
+      Error = Class.new(StandardError)
+      # Do not print depracation warnings; the CLI will do that
+      Gem::Deprecate.skip = true


Do we want to turn this false after executing the operations?

I'm asking you because we're using namespaces and there are a lot of deprecation warnings. Are we assuming that users will always use the cli?

Do we want to turn this false after executing the operations?

Thanks! This makes perfect sense to me, I had missed that. I'll make the change. ⭐️

Background

Good call here, the intention was to disable the default deprecation warnings only in the context of the CLI. since it checks for deprecated methods and marks them.

Previously, we'd see the full default deprecation print out after the CLI exited as capture in https://github.com/akabiru/fakerbot/issues/7 - thus the need to gracefully handle the deprecation warnings (in the context of the CLI)

vbrazo · 2019-02-16T23:31:43Z

faker.gemspec

+  spec.add_dependency('thor', '~> 0.20.0')
+  spec.add_dependency('tty-pager', '~> 0.12.0')
+  spec.add_dependency('tty-screen', '~> 0.6.5')
+  spec.add_dependency('tty-tree', '~> 0.2.0')


Adding these dependencies is a pretty big deal. Are we comfortable with them? I saw the terminal response. It's pretty cool.

Thoughts? @stympy

That's a valid concern, from my POV, these libraries go a long way in making the CLI interface easier to work with. 🙂 The alternative would be to write it all ourselves which I'm not sure about

FWIW, I find all these dependencies to be a big deal. I opened an issue about it (#1642) and plan to stay on 1.9.3 for the time being.

@myronmarston your concern is valid.

@akabiru would you agree to remove these dependencies and just show a formatted list of searched results?

Hey folks, apologies for the very delayed response (on vacation). I think the move to a separate gem is the sensible approach. And allows users to opt-in into using the gem.

It did start as a separate gem , no strong objections on this one. 🙂

@vbrazo I'm happy to assist with the extraction &/migration as well.

* move cli doc to unreleased * Reinstate rubygems deprecation warnings

akabiru · 2019-02-18T19:01:05Z

lib/cli.rb

+    class Base < Thor
+      Error = Class.new(StandardError)
+      # Skip default deprecation warning output; the CLI will display that.
+      Gem::Deprecate.skip_during do


Wrapping the CLI methods in Gem::Deprecate.skip_during block ensures we safely skip the warnings only in the CLI. Although it's intended for use in tests only, it seems like a sensible solution to me. Ref. https://ruby-doc.org/stdlib-1.9.3/libdoc/rubygems/rdoc/Gem/Deprecate.html

akabiru · 2019-03-03T12:45:56Z

Thank you Vitor. Looking forward to continuously making it even better as a community. 🙂

* feat(cli): Pull in fakerbot gem logic * chore(cli): Extract module outside of `faker` dir The module dir `faker` houses the main "faker" namespaces; and it should remain that way. Best to have the CLI live outside the directory; similar to helpers * feat(cli): Add `faker` excecutable * feat(cli): Add renderer; reflector unit tests * feat(cli): Add commands tests * feat(cli): Add integration tests * feat(cli): Add documentation 📖 * chore(cli): Extract constructor to base command * fix(cli): Amend faker version load path - Add integration test to cover that * fix(cli): Skip default deprecation warning output in the CLI * chore(cli): Amend var names * chore(reflectors): Distribute Reflector responsibilities The Reflector was doing too much; handling Search and List reflections which made it a tad harder to follow. This commit defines a Reflection interface and the correspoding subclasses. * chore: Don't specify bunlder version There's an erroneous 'null byte' error in the CI which could be caused by the bundler version. Further, As of Ruby 2.6.0, bundler is now built into Ruby, so no need to install it ourselves * chore: Address review comments * move cli doc to unreleased * Reinstate rubygems deprecation warnings * chore: Skip Gem::Deprecate warning output in CLI

akabiru changed the title ~~Add CLI - Integrate fakerbot~~ Add CLI - Integrate fakerbot 🤖 Jan 5, 2019

vbrazo assigned vbrazo and akabiru and unassigned vbrazo Jan 5, 2019

vbrazo added the 💡 Issue: Feature Request label Jan 5, 2019

akabiru added 11 commits January 5, 2019 22:37

feat(cli): Pull in fakerbot gem logic

e34a04b

chore(cli): Extract module outside of faker dir

552de6e

The module dir `faker` houses the main "faker" namespaces; and it should remain that way. Best to have the CLI live outside the directory; similar to helpers

feat(cli): Add faker excecutable

2582f5d

feat(cli): Add renderer; reflector unit tests

38ec79c

feat(cli): Add commands tests

c5e7224

feat(cli): Add integration tests

5a65094

feat(cli): Add documentation 📖

deb31a4

chore(cli): Extract constructor to base command

96b2dd1

fix(cli): Amend faker version load path

a4b2bc2

- Add integration test to cover that

fix(cli): Skip default deprecation warning output in the CLI

9285086

chore(cli): Amend var names

63a24f1

akabiru mentioned this pull request Jan 14, 2019

CLI interface for faker #1278

Closed

4 tasks

chore(reflectors): Distribute Reflector responsibilities

fd9d986

The Reflector was doing too much; handling Search and List reflections which made it a tad harder to follow. This commit defines a Reflection interface and the correspoding subclasses.

akabiru added 3 commits February 13, 2019 23:12

Merge master

203ef41

Merge branch 'master' into feature/pull-in-fakerbot

450dbaf

chore: Don't specify bunlder version

a800621

There's an erroneous 'null byte' error in the CI which could be caused by the bundler version. Further, As of Ruby 2.6.0, bundler is now built into Ruby, so no need to install it ourselves

vbrazo reviewed Feb 16, 2019

View reviewed changes

akabiru added 2 commits February 18, 2019 02:03

chore: Address review comments

7ca6e5d

* move cli doc to unreleased * Reinstate rubygems deprecation warnings

chore: Skip Gem::Deprecate warning output in CLI

491b63a

akabiru commented Feb 18, 2019

View reviewed changes

vbrazo approved these changes Mar 3, 2019

View reviewed changes

vbrazo merged commit 2ef261c into faker-ruby:master Mar 3, 2019

akabiru deleted the feature/pull-in-fakerbot branch March 3, 2019 12:41

richardbulger mentioned this pull request Mar 7, 2019

Namespace missing in fakerbot search results #1565

Closed

This was referenced Mar 9, 2019

Add deprecation notes; thank you contributors! 🎉 akabiru/faker-bot#25

Merged

Should search include namespaces in addition to methods? akabiru/faker-bot#8

Closed

myronmarston mentioned this pull request Jun 24, 2019

Faker 1.9.4 has so many more dependencies than Faker 1.9.3. #1642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI - Integrate fakerbot 🤖 #1507

Add CLI - Integrate fakerbot 🤖 #1507

akabiru commented Jan 5, 2019 •

edited

stympy commented Jan 5, 2019

akabiru commented Jan 5, 2019

akabiru commented Jan 13, 2019 •

edited

akabiru commented Jan 22, 2019 •

edited

vbrazo commented Jan 24, 2019

akabiru commented Jan 24, 2019

vbrazo left a comment

vbrazo Feb 9, 2019 •

edited

akabiru Feb 17, 2019

vbrazo Feb 18, 2019

akabiru Feb 18, 2019

vbrazo Feb 16, 2019

akabiru Feb 17, 2019

vbrazo Feb 16, 2019

akabiru Feb 17, 2019 •

edited

vbrazo Feb 16, 2019

akabiru Feb 17, 2019

myronmarston Jun 24, 2019

vbrazo Jun 24, 2019

akabiru Jul 5, 2019

akabiru Feb 18, 2019

akabiru commented Mar 3, 2019 •

edited

Navigation Menu

Add CLI - Integrate fakerbot 🤖 #1507

Add CLI - Integrate fakerbot 🤖 #1507

Conversation

akabiru commented Jan 5, 2019 • edited

What does this PR do?

How should this be manually tested?

What are the relevant GitHub issues?

Questions:

Screenshots

stympy commented Jan 5, 2019

akabiru commented Jan 5, 2019

akabiru commented Jan 13, 2019 • edited

akabiru commented Jan 22, 2019 • edited

vbrazo commented Jan 24, 2019

akabiru commented Jan 24, 2019

vbrazo left a comment

Choose a reason for hiding this comment

vbrazo Feb 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akabiru Feb 17, 2019 • edited

Choose a reason for hiding this comment

Background

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akabiru commented Mar 3, 2019 • edited

akabiru commented Jan 5, 2019 •

edited

akabiru commented Jan 13, 2019 •

edited

akabiru commented Jan 22, 2019 •

edited

vbrazo Feb 9, 2019 •

edited

akabiru Feb 17, 2019 •

edited

akabiru commented Mar 3, 2019 •

edited