Re: Review Request: CrawlerBeansPropHandler doesn't set list properties for Spring PropertyOverrideConfigurer correctly

[brian Foster]

> On 2012-03-26 14:22:31, Chris Mattmann wrote:
> > trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java,
> >  line 60
> > 
> >
> > Agree with Tom here, do we need a check on if getValues() == null too?

i think what i'll do here is check if it is empty and then throw an Exception 
explicitly so it is clear that a no arg option it wrong here... boolean options 
should have a static arg specifying either true or false value.


- brian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4454/#review6347
---


On 2012-03-22 21:27:55, brian Foster wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4454/
> ---
> 
> (Updated 2012-03-22 21:27:55)
> 
> 
> Review request for oodt, Chris Mattmann, Ricky Nguyen, Paul Ramirez, and 
> Thomas Bennett.
> 
> 
> Summary
> ---
> 
> It currently was setting lists keys to: value1,value2,value3,...
> 
> This patch will instead change it to:
> key[0] = value1
> key[1] = value2
> 
> 
> This addresses bug OODT-428.
> https://issues.apache.org/jira/browse/OODT-428
> 
> 
> Diffs
> -
> 
>   
> trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java
>  1304041 
> 
> Diff: https://reviews.apache.org/r/4454/diff
> 
> 
> Testing
> ---
> 
> still need a unit-test for this... will write when i get the time
> 
> 
> Thanks,
> 
> brian
> 
>

Re: Review Request: CrawlerBeansPropHandler doesn't set list properties for Spring PropertyOverrideConfigurer correctly

[brian Foster]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4454/
---

(Updated 2012-03-26 20:55:22.461160)


Review request for oodt, Chris Mattmann, Ricky Nguyen, Paul Ramirez, and Thomas 
Bennett.


Changes
---

- For case optionInstance.getValues().isEmpty() now throws RuntimeException
- Added Unit-tests


Summary
---

It currently was setting lists keys to: value1,value2,value3,...

This patch will instead change it to:
key[0] = value1
key[1] = value2


This addresses bug OODT-428.
https://issues.apache.org/jira/browse/OODT-428


Diffs (updated)
-

  
trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java
 1304041 
  
trunk/crawler/src/test/org/apache/oodt/cas/crawl/cli/option/handler/TestCrawlerBeansPropHandler.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/4454/diff


Testing (updated)
---

Wrote unit-test


Thanks,

brian

Re: Review Request: CrawlerBeansPropHandler doesn't set list properties for Spring PropertyOverrideConfigurer correctly

[Chris Mattmann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4454/#review6386
---

Ship it!


LGTM.

- Chris


On 2012-03-26 20:55:22, brian Foster wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4454/
> ---
> 
> (Updated 2012-03-26 20:55:22)
> 
> 
> Review request for oodt, Chris Mattmann, Ricky Nguyen, Paul Ramirez, and 
> Thomas Bennett.
> 
> 
> Summary
> ---
> 
> It currently was setting lists keys to: value1,value2,value3,...
> 
> This patch will instead change it to:
> key[0] = value1
> key[1] = value2
> 
> 
> This addresses bug OODT-428.
> https://issues.apache.org/jira/browse/OODT-428
> 
> 
> Diffs
> -
> 
>   
> trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java
>  1304041 
>   
> trunk/crawler/src/test/org/apache/oodt/cas/crawl/cli/option/handler/TestCrawlerBeansPropHandler.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/4454/diff
> 
> 
> Testing
> ---
> 
> Wrote unit-test
> 
> 
> Thanks,
> 
> brian
> 
>

Re: Review Request: CrawlerBeansPropHandler doesn't set list properties for Spring PropertyOverrideConfigurer correctly

[Chris Mattmann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4454/#review6380
---

Ship it!


Sounds good LGTM.

- Chris


On 2012-03-26 20:55:22, brian Foster wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4454/
> ---
> 
> (Updated 2012-03-26 20:55:22)
> 
> 
> Review request for oodt, Chris Mattmann, Ricky Nguyen, Paul Ramirez, and 
> Thomas Bennett.
> 
> 
> Summary
> ---
> 
> It currently was setting lists keys to: value1,value2,value3,...
> 
> This patch will instead change it to:
> key[0] = value1
> key[1] = value2
> 
> 
> This addresses bug OODT-428.
> https://issues.apache.org/jira/browse/OODT-428
> 
> 
> Diffs
> -
> 
>   
> trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java
>  1304041 
>   
> trunk/crawler/src/test/org/apache/oodt/cas/crawl/cli/option/handler/TestCrawlerBeansPropHandler.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/4454/diff
> 
> 
> Testing
> ---
> 
> Wrote unit-test
> 
> 
> Thanks,
> 
> brian
> 
>

Re: Review Request: CrawlerBeansPropHandler doesn't set list properties for Spring PropertyOverrideConfigurer correctly

[Chris Mattmann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4454/#review6347
---


LGTM, see my comment about the null?


trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java


Agree with Tom here, do we need a check on if getValues() == null too?


- Chris


On 2012-03-22 21:27:55, brian Foster wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4454/
> ---
> 
> (Updated 2012-03-22 21:27:55)
> 
> 
> Review request for oodt, Chris Mattmann, Ricky Nguyen, Paul Ramirez, and 
> Thomas Bennett.
> 
> 
> Summary
> ---
> 
> It currently was setting lists keys to: value1,value2,value3,...
> 
> This patch will instead change it to:
> key[0] = value1
> key[1] = value2
> 
> 
> This addresses bug OODT-428.
> https://issues.apache.org/jira/browse/OODT-428
> 
> 
> Diffs
> -
> 
>   
> trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java
>  1304041 
> 
> Diff: https://reviews.apache.org/r/4454/diff
> 
> 
> Testing
> ---
> 
> still need a unit-test for this... will write when i get the time
> 
> 
> Thanks,
> 
> brian
> 
>

Re: Review Request: CrawlerBeansPropHandler doesn't set list properties for Spring PropertyOverrideConfigurer correctly

[Thomas Bennett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4454/#review6279
---

Ship it!


I've tested it. Looks good to me!


trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java


May need to test for boolean optionsInstances, i.e. where 
optionInstance.getValues().size() == 0.


- Thomas


On 2012-03-22 21:27:55, brian Foster wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4454/
> ---
> 
> (Updated 2012-03-22 21:27:55)
> 
> 
> Review request for oodt, Chris Mattmann, Ricky Nguyen, Paul Ramirez, and 
> Thomas Bennett.
> 
> 
> Summary
> ---
> 
> It currently was setting lists keys to: value1,value2,value3,...
> 
> This patch will instead change it to:
> key[0] = value1
> key[1] = value2
> 
> 
> This addresses bug OODT-428.
> https://issues.apache.org/jira/browse/OODT-428
> 
> 
> Diffs
> -
> 
>   
> trunk/crawler/src/main/java/org/apache/oodt/cas/crawl/cli/option/handler/CrawlerBeansPropHandler.java
>  1304041 
> 
> Diff: https://reviews.apache.org/r/4454/diff
> 
> 
> Testing
> ---
> 
> still need a unit-test for this... will write when i get the time
> 
> 
> Thanks,
> 
> brian
> 
>