- Download Diff

Summary:

[Shadow] add CliRequest class

Review Request #88 — Created April 25, 2013 and submitted May 6, 2013, 2:45 p.m.

Information
Owner:	olivier
Repository:	Lunr
Branch:	https://github.com/owizen/lunr/tree/CliRequest
Bugs:	7315
Depends On:
Reviewers
Groups:	lunr
People:

Description

this adds the Cli request class made for being compatible in signature with the request class.

Testing Done

Unit tests prepared but waiting for GetoptCliParser

Issues

0
0
23
0
All Issues: 23

Description	From	Last Updated
What's this used for?	pprkut	April 25, 2013, 6:13 p.m.
I'm expecting 10 entries here, but there are only 2	pprkut	April 25, 2013, 6:13 p.m.
What's a stored value and why would it be empty?	pprkut	April 25, 2013, 6:13 p.m.
Implementation doesn't match documentation. At all.	pprkut	April 25, 2013, 6:12 p.m.
this is void. Why does it return NULL? Why is it not implemented properly?	pprkut	April 25, 2013, 6:13 p.m.
unrelated change	pprkut	May 2, 2013, 1:45 p.m.
This does not do what it advertises	pprkut	May 2, 2013, 3:09 p.m.
you are aware that what you are doing here is like asking santa clause if his first name is santa, ...	pprkut	May 3, 2013, 11:56 a.m.
this tries to do too much. Split it up	pprkut	May 2, 2013, 4:01 p.m.
why the extra parameters?	pprkut	May 3, 2013, 9:25 a.m.
mock the configuration class	pprkut	May 2, 2013, 4:01 p.m.
Semantics: -We have a certain format for methods that return data from an environment source. All of have a name ...	pprkut	May 6, 2013, 9:39 a.m.
I still don't know why you treat an empty array as a special value. If you look at the function ...	pprkut	May 6, 2013, 9:39 a.m.
This kind of hangs in the air, compared to the other methods. There is no equivalent for post/get/cookie and the ...	pprkut	May 6, 2013, 10:10 a.m.
I still don't understand why it hurts to implement this	pprkut	May 6, 2013, 10:10 a.m.
Please use meaningful test names. phpunit has an option that translates the method name into a test description. This one ...	pprkut	May 6, 2013, 10:15 a.m.
If you look at this you see that what you are doing here is actually performing five tests in one. ...	pprkut	May 6, 2013, 10:15 a.m.
That is correct, but non-constructive. You write unit tests only for the purpose of code coverage. But if you look ...	pprkut	May 6, 2013, 11:54 a.m.
will it stay like this?	dinos	May 6, 2013, 2:20 p.m.
here too	dinos	May 6, 2013, 2:20 p.m.
here too	dinos	May 6, 2013, 2:20 p.m.
here too	dinos	May 6, 2013, 2:20 p.m.
here too	dinos	May 6, 2013, 2:20 p.m.

Change Summary:

Add magic getter + "unit tests"

Diff:

Revision 2 (+481)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php

src/Lunr/Shadow/CliRequest.php (Diff revision 2)

Show all issues

What's this used for?

olivier April 26, 2013, 7:32 p.m.

for early exit if the parser detects invalid command line

pprkut April 27, 2013, 8:39 p.m.
```
and you do that where exactly?
```
olivier April 27, 2013, 9:02 p.m.
```
In the front controller, for example
```

pprkut April 28, 2013, 1:03 p.m.

What's the use case for it? When would an application developer want to know that the command line was invalid? What would he do with that information?

src/Lunr/Shadow/CliRequest.php (Diff revision 2)

Show all issues

I'm expecting 10 entries here, but there are only 2

olivier April 26, 2013, 7:32 p.m.

I can then init the entries with all the default entries from the configuratio

pprkut April 27, 2013, 8:39 p.m.
```
why would you do that?
```
olivier April 27, 2013, 9 p.m.
```
for class signature compatibility.
```

pprkut April 28, 2013, 1:03 p.m.

Check the Request class. When are the values initialized with the default values and what's the impact of that on the CliRequest class?

src/Lunr/Shadow/CliRequest.php (Diff revision 2)

Show all issues

What's a stored value and why would it be empty?

olivier April 26, 2013, 7:32 p.m.

following this command: -a param -b

 get_arg('a') returns "param"
 get_arg('b') returns false cause the key contains an empty array -> 0 in an invalid index

olivier April 27, 2013, 9 p.m.

Note I am not sure that false is the best value for empty keys.

pprkut April 28, 2013, 1:03 p.m.

Consider a developer who wants to do something with the arguments. He finds a function that says "Retrieve a stored value for..." and his first reaction is "wtf is a stored value..."

olivier April 29, 2013, 4:41 p.m.

I got it, the diff was updated this way.

src/Lunr/Shadow/CliRequest.php (Diff revision 2)

Show all issues

Implementation doesn't match documentation. At all.

olivier April 26, 2013, 7:32 p.m.
```
Oops crappy copy paste
```

src/Lunr/Shadow/CliRequest.php (Diff revision 2)

Show all issues

this is void. Why does it return NULL? Why is it not implemented properly?

olivier April 26, 2013, 7:32 p.m.
```
same as before
```

Change Summary:

change made according to review
unit tests added, more to come concerning them.

Diff:

Revision 3 (+528)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

Change Summary:

Update unit tests

Diff:

Revision 4 (+548 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

Change Summary:

Minor update

Diff:

Revision 5 (+548 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

Change Summary:

Update made to class according to review.
Introduce store_url and store_get methods.
Unit tests updated, return statement added to LunrBaseTest::get_reflection_property_value

Diff:

Revision 6 (+577 -2)

Show changes

	src/Lunr/Halo/LunrBaseTest.php
	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

src/Lunr/Halo/LunrBaseTest.php (Diff revision 6)

Show all issues

unrelated change

olivier May 2, 2013, 3:11 p.m.

I know, should dispappear once this is merged
http://reviews.lunr.nl/r/91/

pprkut May 2, 2013, 5:41 p.m.

No. Properly base your diff against the branch this is based on and document your dependencies. I will reject double reviews. No exception.

src/Lunr/Shadow/CliRequest.php (Diff revision 6)

Show all issues

This does not do what it advertises

src/Lunr/Shadow/CliRequest.php (Diff revision 6)

Show all issues

you are aware that what you are doing here is like asking santa clause if his first name is santa, right?

olivier May 2, 2013, 3:11 p.m.

I know but other wise I dont have any point in making the method and removing the affectations from the constructor...

olivier May 2, 2013, 5:09 p.m.

And I would add that the request class checks the sapi to act, I update the review and we will discuss about it later

pprkut May 2, 2013, 5:41 p.m.

Dude, cause and effect. Simple physics. Cause = Disussion, Effect = Change. No change without discussion. What do you change? You haven't understood anything...

olivier May 3, 2013, 9:26 a.m.

No and no clear point of view for me concerning this...

pprkut May 3, 2013, 11:25 a.m.

Ok, in simple terms then. You have a class abstracting a CLI request. And you have a function that does things differently depending on whether the request was made from the cli or elsewhere. What use case do you see where the CLI request class is used in a non-cli context?

olivier May 3, 2013, 11:57 a.m.
```
So I get it
```

src/Lunr/Shadow/Tests/CliRequestTest.php (Diff revision 6)

Show all issues

this tries to do too much. Split it up

olivier May 2, 2013, 3:11 p.m.

Ok will be done so
Let's say I add base and get set files

src/Lunr/Shadow/Tests/CliRequestTest.php (Diff revision 6)

Show all issues

why the extra parameters?

pprkut May 2, 2013, 5:41 p.m.

Sigh. I'm ASKING a QUESTION, god dammit.

olivier May 3, 2013, 9:26 a.m.

Cause I did't use the mock builder, this is changed by now. Otherwise invoking the parse method failed
and the mock didn't build without contructor params.

src/Lunr/Shadow/Tests/CliRequestTest.php (Diff revision 6)
Show all issues
```
mock the configuration class
```

Change Summary:

Change made according to review,
split the test case in 3 files.
for store_url,  dont know what is the best action.

Diff:

Revision 7 (+654 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestBaseTest.php
	src/Lunr/Shadow/Tests/CliRequestGetSetTest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

Change Summary:

Modifiy store_url method

Diff:

Revision 8 (+627 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestBaseTest.php
	src/Lunr/Shadow/Tests/CliRequestGetSetTest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

Change Summary:

Remove unproper comments in CliRequestTest file

Diff:

Revision 9 (+617 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestBaseTest.php
	src/Lunr/Shadow/Tests/CliRequestGetSetTest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

src/Lunr/Shadow/CliRequest.php (Diff revision 9)

Conceptually this is fine. And if it were the only thing left, I wouldn't mind it staying that way. But if you have to go over it one more time, you might just as well combine it.

olivier May 6, 2013, 10:23 a.m.

I will group them in store default as it is the only thing we do here in fact.

src/Lunr/Shadow/CliRequest.php (Diff revision 9)

Show all issues

Semantics:

-We have a certain format for methods that return data from an environment source. All of have a name matching "get_..._data", except for this one.

- On the cli we have parameters/arguments and options. Looks like I was using them wrongly too (was looking it up just now). If we look at the command "cli.php -a test", then "-a" is an option and "test" is an argument/parameter to that option. I don't mind whether you use parameter or argument for it, but be consistent and correct, ie. don't say argument when you mean option.

- $index is no longer necessary

olivier May 6, 2013, 10:23 a.m.

go for get_option_data as it is the most consistent name we can give here.

src/Lunr/Shadow/CliRequest.php (Diff revision 9)

Show all issues

I still don't know why you treat an empty array as a special value. If you look at the function from a semantic view, "get_arg" returning "FALSE" is an error case. But an option with no argument is not necessarily an error

olivier May 6, 2013, 10:23 a.m.

I think the best option would be to return an empty array then.

src/Lunr/Shadow/CliRequest.php (Diff revision 9)

Show all issues

This kind of hangs in the air, compared to the other methods. There is no equivalent for post/get/cookie and the function name doesn't match the documentation.
If you can think of (please do, because I might be overlooking something) a good use case for having this it can stay (with proper function name). Otherwise I'm inclined to just see it gone.

olivier May 6, 2013, 10:23 a.m.

then renaming it to get_all_options seems a correct name.

src/Lunr/Shadow/CliRequest.php (Diff revision 9)

Show all issues

I still don't understand why it hurts to implement this

olivier May 6, 2013, 10:23 a.m.

True this can be usefull and doesn't really cost time

src/Lunr/Shadow/Tests/CliRequestBaseTest.php (Diff revision 9)

Show all issues

Please use meaningful test names. phpunit has an option that translates the method name into a test description. This one would say:

test Store url........OK/FAILED

which doesn't really tell me anything about the test (except that I'm storing the URL. For a CLI request)

src/Lunr/Shadow/Tests/CliRequestBaseTest.php (Diff revision 9)

Show all issues

If you look at this you see that what you are doing here is actually performing five tests in one. That's something you want to avoid generally and what we use data providers for.

src/Lunr/Shadow/Tests/CliRequestGetSetTest.php (Diff revision 9)

Show all issues

That is correct, but non-constructive. You write unit tests only for the purpose of code coverage. But if you look at the implementation in the standard request class you'll see that there is an actual useful test you can implement that tests the same thing, but doesn't need to be rewritten when we have the time to properly implement this.

(also valid for the other NULL methods)

Change Summary:

Change made according to review, implement json_enums support.

Diff:

Revision 10 (+768 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestBaseTest.php
	src/Lunr/Shadow/Tests/CliRequestGetSetTest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

Change Summary:

change made on get_option_data and unit tests

Diff:

Revision 11 (+821 -1)

Show changes

	src/Lunr/Shadow/CliRequest.php
	src/Lunr/Shadow/Tests/CliRequestBaseTest.php
	src/Lunr/Shadow/Tests/CliRequestGetTest.php
	src/Lunr/Shadow/Tests/CliRequestTest.php
	tests/phpunit.xml

src/Lunr/Shadow/CliRequest.php (Diff revision 11)

Show all issues

will it stay like this?

olivier May 6, 2013, 2:20 p.m.

That method is here for signature compatibility with the core Request class.
Both CliRequest and Request classes must be exchangeable. In the future
this might return something, if there is a relevant usecase.

src/Lunr/Shadow/CliRequest.php (Diff revision 11)
Show all issues
```
here too
```
1. olivier May 6, 2013, 2:20 p.m.
  same
src/Lunr/Shadow/CliRequest.php (Diff revision 11)
Show all issues
```
here too
```
1. olivier May 6, 2013, 2:20 p.m.
  same
src/Lunr/Shadow/CliRequest.php (Diff revision 11)
Show all issues
```
here too
```
1. olivier May 6, 2013, 2:20 p.m.
  same
src/Lunr/Shadow/CliRequest.php (Diff revision 11)
Show all issues
```
here too
```
1. olivier May 6, 2013, 2:20 p.m.
  same

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Shadow] add CliRequest class

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: