Misc fixes. #18

Merged
merged 3 commits into from Mar 21, 2013

Conversation

Projects
None yet
2 participants
@jeddenlea
  • Fix spot-price-history.
  • Further fix output of spot-check-history and display-types;
    cl-format doesn't flush out.
  • Clean up shell_test. It was broken with JAVA_HOMEs like
    "/usr/lib/jvm/java-6-sun/jre".

Jed Denlea added some commits Mar 20, 2013

Jed Denlea
Lemur fixes.
- Fix spot-check-history.
- Further fix output of spot-check-history and display-types;
  cl-format doesn't flush *out*.
- Clean up shell_test. It was broken with JAVA_HOMEs like
  "/usr/lib/jvm/java-6-sun/jre".
Jed Denlea
Quick follow-up.
Condense test-merge-env-2.  I'm still getting the hang of this Clojure deal.
@@ -902,7 +902,8 @@ calls launch - take action (upload files, start cluster, etc)
(cl-format true "~{~12<~a~>~}~%~
~{~{~12<~a~>~}~%~}"
(take-nth 2 flds)
- details))
+ details)
+ (flush))

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

fix indentation

@mlimotte

mlimotte Mar 21, 2013

Contributor

fix indentation

This comment has been minimized.

Show comment Hide comment
@jeddenlea

jeddenlea Mar 21, 2013

The indentation here is correct, the (flush) follows (cl-format) within the (let).

@jeddenlea

jeddenlea Mar 21, 2013

The indentation here is correct, the (flush) follows (cl-format) within the (let).

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

Ah. I see it now.

@mlimotte

mlimotte Mar 21, 2013

Contributor

Ah. I see it now.

- start (.getTime (.add now GregorianCalendar/HOUR_OF_DAY hours))
+ (let [[start end] (let [volatile-cal (GregorianCalendar.)
+ end (.getTime volatile-cal)]
+ (.add volatile-cal GregorianCalendar/HOUR_OF_DAY (- hours))

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

Can you explain the purpose of this change?

@mlimotte

mlimotte Mar 21, 2013

Contributor

Can you explain the purpose of this change?

This comment has been minimized.

Show comment Hide comment
@jeddenlea

jeddenlea Mar 21, 2013

In the prior form, running spot-price-history would yield a NullPointeException. This is because GregorianCalendar.add(...) mutates the calendar in place and returns void. Further, adding hours rather than (- hours) would create a window of time starting from now spanning 1 day into the future, which wouldn't create interesting results.

I've tried to stay within the realm of the original intent, using the same functions, but limiting the existence of the volatile calendar object into its own tiny (let).

@jeddenlea

jeddenlea Mar 21, 2013

In the prior form, running spot-price-history would yield a NullPointeException. This is because GregorianCalendar.add(...) mutates the calendar in place and returns void. Further, adding hours rather than (- hours) would create a window of time starting from now spanning 1 day into the future, which wouldn't create interesting results.

I've tried to stay within the realm of the original intent, using the same functions, but limiting the existence of the volatile calendar object into its own tiny (let).

@mlimotte

View changes

src/main/clj/lemur/core.clj
@@ -918,7 +919,8 @@ calls launch - take action (upload files, start cluster, etc)
(cl-format true "~{~12<~a~>~}~%~
~{~{~12<~a~>~}~%~}"
(take-nth 2 flds)
- details))
+ details)
+ (flush))

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

indentation

@mlimotte

mlimotte Mar 21, 2013

Contributor

indentation

This comment has been minimized.

Show comment Hide comment
@jeddenlea

jeddenlea Mar 21, 2013

Fixed.

@mlimotte

View changes

src/test/clj/com/climate/shell_test.clj
- (get (System/getenv) \"FOO\"))"
- :err :pass
- :env (merge-env {:FOO "bar"}))))))
+ (is (when-let [java-home (not-empty (get (System/getenv) "JAVA_HOME"))]

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

You don't need not-empty here. (get) returns nil if there is no entry for JAVA_HOME, which is "falsey" and goo enough got when-let

@mlimotte

mlimotte Mar 21, 2013

Contributor

You don't need not-empty here. (get) returns nil if there is no entry for JAVA_HOME, which is "falsey" and goo enough got when-let

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

More importantly, I don't see how this is better. Can you explain the problem with the original impl?

From your log comment:
It was broken with JAVA_HOMEs like "/usr/lib/jvm/java-6-sun/jre"
I think the issue is that I didn't consider "-". Adding hyphen to the regex fixes the problem:
#"["[-./\w]+" "bar"]"

@mlimotte

mlimotte Mar 21, 2013

Contributor

More importantly, I don't see how this is better. Can you explain the problem with the original impl?

From your log comment:
It was broken with JAVA_HOMEs like "/usr/lib/jvm/java-6-sun/jre"
I think the issue is that I didn't consider "-". Adding hyphen to the regex fixes the problem:
#"["[-./\w]+" "bar"]"

This comment has been minimized.

Show comment Hide comment
@jeddenlea

jeddenlea Mar 21, 2013

While we could add a hyphen to the regular expression, it seems sloppy to use a regular expression to test if a known value is present. What this test is doing is taking the existing known environment, attempting to create a new one, and proving that this new environment is useful to child processes. Consider, in some pathological case, what if merge-env actually corrupted all of the existing environment entries? So, to that end, I think this is a more meaningful approach, testing explicitly whether or not our new env is in fact just like our old one with some new values added.

@jeddenlea

jeddenlea Mar 21, 2013

While we could add a hyphen to the regular expression, it seems sloppy to use a regular expression to test if a known value is present. What this test is doing is taking the existing known environment, attempting to create a new one, and proving that this new environment is useful to child processes. Consider, in some pathological case, what if merge-env actually corrupted all of the existing environment entries? So, to that end, I think this is a more meaningful approach, testing explicitly whether or not our new env is in fact just like our old one with some new values added.

This comment has been minimized.

Show comment Hide comment
@mlimotte

mlimotte Mar 21, 2013

Contributor

Ok. That sounds reasonable.

On Thu, Mar 21, 2013 at 1:43 PM, Jed Denlea notifications@github.comwrote:

In src/test/clj/com/climate/shell_test.clj:

@@ -39,14 +39,15 @@
:env (merge-env {:FOO "bar"}))))))

(deftest test-merge-env-2

  • (is (re-find
  •    #"[\"[.\/\w]+\" \"bar\"]"
    
  •    (:out (sh "java" "-cp" (clj-main-jar) "clojure.main" "-e"
    
  •          "(vector
    
  •             (get (System/getenv) \"JAVA_HOME\")
    
  •             (get (System/getenv) \"FOO\"))"
    
  •          :err :pass
    
  •          :env (merge-env {:FOO "bar"}))))))
    
  • (is (when-let [java-home (not-empty (get (System/getenv) "JAVA_HOME"))]

While we could add a hyphen to the regular expression, it seems sloppy to
use a regular expression to test if a known value is present. What this
test is doing is taking the existing known environment, attempting to
create a new one, and proving that this new environment is useful to child
processes. Consider, in some pathological case, what if merge-env actually
corrupted all of the existing environment entries? So, to that end, I think
this is a more meaningful approach, testing explicitly whether or not our
new env is in fact just like our old one with some new values added.


Reply to this email directly or view it on GitHubhttps://github.com/TheClimateCorporation/lemur/pull/18/files#r3475554
.

@mlimotte

mlimotte Mar 21, 2013

Contributor

Ok. That sounds reasonable.

On Thu, Mar 21, 2013 at 1:43 PM, Jed Denlea notifications@github.comwrote:

In src/test/clj/com/climate/shell_test.clj:

@@ -39,14 +39,15 @@
:env (merge-env {:FOO "bar"}))))))

(deftest test-merge-env-2

  • (is (re-find
  •    #"[\"[.\/\w]+\" \"bar\"]"
    
  •    (:out (sh "java" "-cp" (clj-main-jar) "clojure.main" "-e"
    
  •          "(vector
    
  •             (get (System/getenv) \"JAVA_HOME\")
    
  •             (get (System/getenv) \"FOO\"))"
    
  •          :err :pass
    
  •          :env (merge-env {:FOO "bar"}))))))
    
  • (is (when-let [java-home (not-empty (get (System/getenv) "JAVA_HOME"))]

While we could add a hyphen to the regular expression, it seems sloppy to
use a regular expression to test if a known value is present. What this
test is doing is taking the existing known environment, attempting to
create a new one, and proving that this new environment is useful to child
processes. Consider, in some pathological case, what if merge-env actually
corrupted all of the existing environment entries? So, to that end, I think
this is a more meaningful approach, testing explicitly whether or not our
new env is in fact just like our old one with some new values added.


Reply to this email directly or view it on GitHubhttps://github.com/TheClimateCorporation/lemur/pull/18/files#r3475554
.

mlimotte added a commit that referenced this pull request Mar 21, 2013

@mlimotte mlimotte merged commit d537c99 into TheClimateCorporation:master Mar 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment