Remember command's optoins #60

andriiNyzhnik · 2014-07-31T14:23:56Z

Store options in file in home folder.
Use just '.tddium' file, because when I save options and then user enter command without them I can't guess from what host use config file (e.x. '.tddium' or '.tddium.localhost').

https://bugs.tddium.com/bugzilla/show_bug.cgi?id=619

semipermeable · 2014-07-31T15:45:19Z

"set_params" is a confusing subcommand name.
The word "params" is generic, and can mean vastly different things depending on context.

How about instead:

tddium server  # displays the saved connection info
tddium server:set HOST [--port PORT] [--insecure]  # saves connection info
(port defaults to 443, insecure defaults to off)

andriiNyzhnik · 2014-08-01T10:08:57Z

I have made changes. Now you can review please.

semipermeable · 2014-08-01T14:27:23Z

lib/tddium/cli/commands/server.rb

+      self.class.display
+    end
+
+    desc 'server:set HOST [PORT] [PROTO] [INSECURE]', "saves connection info"


This description does not match the implementation. The options in the description are all positional, but as you havr defined the thor task, they need to be passed with flags.

Oh...Options should be with flags, or without?

It's fine either with flags or without.

The problem is that method_option in thor uses flags ( tddium server:set --host HOST [--port PORT] [--proto PROTO] [--insecure]) but the description on line 11 says that the command takes positional parameters without any flags (tddium server:set HOST [PORT] [PROTO] [INSECURE]).

What happens when you run:

bin/tddium server:set foo.example.com 2345 https false

I'm pretty sure it won't actually save any settings, because options will be empty.

semipermeable · 2014-08-01T14:29:15Z

Needs a test for the server and server commands themselves.

semipermeable · 2014-08-01T14:30:00Z

lib/tddium/constant.rb

@@ -270,6 +272,8 @@ module Process
 Extract details of a session with `tddium describe <session_id>`.

 EOF
+      OPTIONS_SAVED = 'Options have been successfully saved.'
+      NOT_SAVED_OPTIONS = 'You do not have file with options. Run `tddium server:add`.'


Should be "tddium server:set" ?

andriiNyzhnik · 2014-08-04T12:09:00Z

added tests for server commands

semipermeable · 2014-08-04T17:48:06Z

spec/commands/server_spec.rb

+    let(:options){ {host: 'localhost', port: 3000, proto: 'http', insecure: true} }
+
+    it 'stores options and returns successfully message' do
+      subject.stub('server:set').with(options).and_return(self.class::Text::Process::OPTIONS_SAVED)


I don't think is actually testing anything.

(that is: if you were to change the code in cli/commands/server.rb to be broken, this test would still pass)

Yes, but I have also test in spec/params_helper_spec.rb. I forgot to change file name and also there is one extra line in code(I will remove it)

The problem with removing the test is that there's then nothing that actually validates the end-to-end functionality of setting the server.

Ok. Now I just need to add additional tests for ParamsHelper module (lib/tddium/cli/params_helper.rb)?

A test for params_helper is not sufficient.

There needs to be a test that calls

Tddium::TddiumCli.send("server:set", options)

And checks either that write_params was called, or that the setting file was actually written out.

andriiNyzhnik · 2014-08-05T17:59:47Z

Changed tests. Please check.

wkj · 2014-08-05T18:14:04Z

lib/tddium/cli/params_helper.rb

+      end
+    rescue Errno::ENOENT => e
+      # when was called from class 'TddiumCli', return {} to use default options
+      if caller[1][/`([^']*)'/, 1] == '<class:TddiumCli>'


Is it better to test to see if Default::PARAMS_PATH exists with File.exists?

You are right. I will also test presence of PARAMS_PATH constant

2014-08-05 21:14 GMT+03:00 wkj notifications@github.com:

In lib/tddium/cli/params_helper.rb:

@@ -0,0 +1,40 @@
+# Copyright (c) 2014 Solano Labs All Rights Reserved
+
+module ParamsHelper

include TddiumConstant

def load_params

begin

File.open(Default::PARAMS_PATH, 'r') do |file|

return JSON.parse file.read

end

rescue Errno::ENOENT => e

# when was called from class 'TddiumCli', return {} to use default options

if caller[1][/`([^']*)'/, 1] == 'class:TddiumCli'

Is it better to test to see if Default::PARAMS_PATH exists with
File.exists?

—
Reply to this email directly or view it on GitHub
https://github.com/solanolabs/tddium/pull/60/files#r15830829.

I think William is using "test" in the sense of "checking the condition" in the code itself.

i.e, instead of rescuing Errno::ENOENT, to use File.exists? explicity:

if File.exists?(Default::PARAMS_PATH) then JSON.parse(File.read(Default::PARAMS_PATH)) else nil end

andriiNyzhnik · 2014-08-06T09:05:37Z

Have made changes. Please check.

server whitch displays saved options; server:set whitch save options in an file;

semipermeable reviewed Aug 1, 2014
View reviewed changes

semipermeable reviewed Aug 4, 2014
View reviewed changes

wkj reviewed Aug 5, 2014
View reviewed changes

wkj force-pushed the bug_619_remember_options branch 3 times, most recently from 08e40cb to 4443f61 Compare August 25, 2014 22:07

Andrii Nyzhnyk and others added 11 commits August 25, 2014 23:06

Added command set_params, that store params in a file

3be9fb9

Now get params fom file if it exists and than by default

116e92f

Covered set_params command by tests and added missing copyrights

ebc7d66

Now there are two comands:

b0686c9

server whitch displays saved options; server:set whitch save options in an file;

added method to display saved options

2e3e8c9

changed description

ab06cc2

writed commands for server commands

40c727f

have made changes, according to pull_request's comments

007f320

changed tests for server command

0a8e620

Added test to check that PARAMS_PATH constant should not be nil

73a883a

Don't walk the stack to determine how to handle missing file.

ff51c86

wkj force-pushed the bug_619_remember_options branch from 4443f61 to ff51c86 Compare August 25, 2014 23:10

wkj merged commit ff51c86 into master Aug 25, 2014

andriiNyzhnik deleted the bug_619_remember_options branch October 6, 2014 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remember command's optoins #60

Remember command's optoins #60

andriiNyzhnik commented Jul 31, 2014

semipermeable commented Jul 31, 2014

andriiNyzhnik commented Aug 1, 2014

semipermeable Aug 1, 2014

andriiNyzhnik Aug 1, 2014

semipermeable Aug 1, 2014

semipermeable commented Aug 1, 2014

semipermeable Aug 1, 2014

andriiNyzhnik commented Aug 4, 2014

semipermeable Aug 4, 2014

andriiNyzhnik Aug 4, 2014

semipermeable Aug 4, 2014

andriiNyzhnik Aug 4, 2014

semipermeable Aug 4, 2014

andriiNyzhnik commented Aug 5, 2014

wkj Aug 5, 2014

andriiNyzhnik Aug 5, 2014

semipermeable Aug 25, 2014

andriiNyzhnik commented Aug 6, 2014

Remember command's optoins #60

Remember command's optoins #60

Conversation

andriiNyzhnik commented Jul 31, 2014

semipermeable commented Jul 31, 2014

andriiNyzhnik commented Aug 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

semipermeable commented Aug 1, 2014

Choose a reason for hiding this comment

andriiNyzhnik commented Aug 4, 2014

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

andriiNyzhnik commented Aug 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andriiNyzhnik commented Aug 6, 2014