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

Restructure commands: Adding get subcommands (WIP) #195

Merged
merged 1 commit into from Jul 12, 2018

Conversation

Projects
None yet
3 participants
@matheussbernardo
Contributor

matheussbernardo commented Jun 4, 2018

Hello @Ana06 @cornelius,

I started to implement the new commands,
I faced a big problem with the tests. I do not fully understand them, It does not seem to have a pattern between all the tests, some use VCR, other mock in a way others in another away, I need help on that.

Regarding the changes I propose a different structure.

According to the thor docs(http://whatisthor.com/) if I want to implement subcommands like (trollolo get SUBCOMMANDS), I need to create another class that inherits from Thor and then point that class from the parent class and wrap everything around the same module.

So basically what I did was, I created a module name TrolloloCLI on cli.rb, then I created a dir named cli and created a class inside named Get, I copied to this new class all the get related commands and renamed it to what was defined on #15.

On the cli.rb file I require the cli/get.rb, also I had to call some methods that were private on the class Cli because of that I changed these methods to be module methods and the @@settings to be a variable of the module, this way I can call them from the new class Get and in the future from the others classes that I intend to do.

What do you guys think? Is this a good way to do the restructure? And about the tests how can I understand them fully and maybe cleanup the test suite? (seems that @manno is working on that #193 PR)

@cornelius

This comment has been minimized.

Member

cornelius commented Jun 5, 2018

I would keep it simple for now and not introduce a new namespace or rename the command class. You could just add a new class CliGet where you define the get subcommand. This can be included in the same way as all other classes in lib/trollolo.rb and the file could also just live in lib/cli_get.rb.

Calling private methods from outside of a class is usually not a great idea. They are private for a reason after all. Maybe you can find a better way?

The existing @@settings variable also is not a great idea. This kind of global state tends to complicate things. This might be a bigger change but I would try to move the code base into a direction where this is not needed anymore but settings are passed explicitly to the classes which need them.

Regarding the tests I would not use VCR anymore. #193 will remove them. For the command line handling it might be best to not rely on any board data and only test that the logic of handling the commands works properly.

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jun 5, 2018

I understand the problem with a new namespace but it was created to fulfill the problem with the @@settings and the private methods, because they need to be available from any classes on that namespace. so that methods are not that private at all when introducing new classes. I will search for a better way, but I'm not sure what is the best way to share these methods and variables around these new classes.

@Ana06

This comment has been minimized.

Member

Ana06 commented Jun 5, 2018

@matheussbernardo regarding the @@setings, I agree with @cornelius that it would be better to move the code base into a direction where this is not needed anymore but settings are passed explicitly to the classes which need them.

Making CliGet a subclass of Cli may be an option if many of the methods there are needed. So that the namespace is not needed. @cornelius what do you think about that?

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jun 5, 2018

Actually @Ana06 I dont think inherit from Cli makes sense because the Cli class has public methods that I do not want to inherit, and the thor documentation uses the subcommands as subclasses from Thor.

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jun 5, 2018

I thought about introducing a new module that implements

def board_id(id_or_alias)
def require_trello_credentials
def process_global_options(options)
def self.settings=(s)
and getters and setters for settings,
then this module is includein the classes that need to use them.

Or maybe use ruby open classes and include the new module on the Thor class so that all the subcommands and Cli get the methods that is needed

@Ana06

This comment has been minimized.

Member

Ana06 commented Jun 6, 2018

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jun 7, 2018

@cornelius @Ana06 What do you guys think about the module?

@Ana06

This comment has been minimized.

Member

Ana06 commented Jun 11, 2018

@matheussbernardo the idea sounds go to me, @cornelius what do you think?

@cornelius

This comment has been minimized.

Member

cornelius commented Jun 11, 2018

Yes, a module sounds good. Give it a try.

lib/cli.rb Outdated
@@ -408,7 +57,229 @@ def require_trello_credentials
# Returns the board_id using id_or_alias. If id_or_alias matches a mapping
# from trollolorc then the mapped id is returned or else the id_or_alias
# is returned.
def board_id(id_or_alias)
def self.board_id(id_or_alias)

This comment has been minimized.

@Ana06

Ana06 Jun 19, 2018

Member

this method was private before and now it is public 🤔

lib/cli.rb Outdated
write_back = false

This comment has been minimized.

@Ana06

Ana06 Jun 19, 2018

Member

extra spaces that can be removed 😉

lib/cli.rb Outdated
@@settings.verbose = options[:verbose]
@@settings.raw = options[:raw]
end
def require_trello_credentials
def self.require_trello_credentials

This comment has been minimized.

@Ana06

Ana06 Jun 19, 2018

Member

this method was private before and now it is public 🤔

lib/cli.rb Outdated
class_option :version, type: :boolean, desc: 'Show version'
class_option :verbose, type: :boolean, desc: 'Verbose mode'
class_option :raw, type: :boolean, desc: 'Raw mode'
# class_option 'board-id', type: :string, desc: 'id of Trello board'

This comment has been minimized.

@Ana06

Ana06 Jun 19, 2018

Member

why is this comment here?

lib/cli.rb Outdated
class_option :verbose, type: :boolean, desc: 'Verbose mode'
class_option :raw, type: :boolean, desc: 'Raw mode'
# class_option 'board-id', type: :string, desc: 'id of Trello board'

This comment has been minimized.

@Ana06

Ana06 Jun 19, 2018

Member

too many empty lines 🙈

lib/cli.rb Outdated
def self.settings=(s)
@@settings = s
end
# class_option 'board-id', type: :string, desc: 'id of Trello board'

This comment has been minimized.

@cornelius

cornelius Jun 21, 2018

Member

Don't leave in code which has been commented out.

@cornelius

This comment has been minimized.

Member

cornelius commented Jun 21, 2018

The code in general looks fine now but please rebase as Ana suggested so we can also see the CI run.

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:master branch 2 times, most recently from 54e59cc to 9e685b6 Jun 25, 2018

@Ana06

Please prepare the commits to be merged with a description. 😉

@@ -96,6 +96,7 @@ Layout/ExtraSpacing:
Layout/IndentHeredoc:
Exclude:
- 'lib/cli.rb'
- 'spec/unit/cli/*.rb'

This comment has been minimized.

@Ana06

Ana06 Jun 25, 2018

Member

you can do this automatically by running: rubocop --auto-gen-conf

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:master branch 2 times, most recently from 1301cc9 to a2c5f6f Jun 28, 2018

@Ana06

Ana06 approved these changes Jul 2, 2018

LGTM, @cornelius please take a look 😉

@cornelius

This comment has been minimized.

Member

cornelius commented Jul 2, 2018

In general the patch looks great now. I would suggest to slightly change the naming of the classes, though, to get a more consistent naming scheme. Sorry, if that feels like nitpicking but it's easier to do this before things are merged and so we get a clean state in master right away.

I would name all the CLI classes with a Cliprefix so CliGet, CliBackupetc. instead of using the suffix. This is more consistent with the directory structure, e.g. cli/get.rb. It also makes sure that when classes or files are sorted alphabetically they all go together. This can be handy when looking at files or in other tools.

It should be Cli, not CLI, in the class names, by the way. The convention is to only capitalize the first letter so that CamelCasing still works.

And there seems to be a test failing in the CI. This should also be fixed.

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:master branch from a2c5f6f to a0891d0 Jul 3, 2018

Restructure commands
* Use Thor subcommands and creates new classes for each new subcommand
* Change commands name to be more expressive
* Change tests to reflect the changes

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:master branch from a0891d0 to 1cef8d7 Jul 11, 2018

@cornelius

This comment has been minimized.

Member

cornelius commented Jul 11, 2018

looks good to me. please merge when the ci is green.

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jul 11, 2018

Ready to merge!

@Ana06

Ana06 approved these changes Jul 12, 2018

Then let's merge it! 😉

@Ana06 Ana06 merged commit 82fd56c into openSUSE:master Jul 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment