Avast ye string handlers

[jcorrado]

Hi @devth,

Friday afternoon puttering! This is partly for the fun of it, and partly to kick around some ideas for #740, and general codebase exploration.

jereme [4:32 PM]
!complete Why is Clojure | head 5 | xargs pirate

yetibot APP [4:32 PM]
why be cllojurre jolllly good
why be clojure popular
why be clojurre dynaamicalllly typed, yo ho ho
why be clojurre so slow, arrrrrrr
why be clojurre not popullaar, llock 'im in irrons

[devth]

Lol! Amazing. I don't see a diff on here yet - just some master branch merges.

[jcorrado]

Hrm, I botched this PR somehow. Looking into it now.

[codecov-io]

Codecov Report

Merging #747 into master will increase coverage by 1.42%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master     #747      +/-   ##
==========================================
+ Coverage   41.67%   43.09%   +1.42%     
==========================================
  Files          79       80       +1     
  Lines        2294     2369      +75     
  Branches       84       84              
==========================================
+ Hits          956     1021      +65     
- Misses       1254     1264      +10     
  Partials       84       84

[jcorrado]

Those pullll requests duty better when ye make some commits on tha brraanch aforre pushing, brring 'er alongside.

Fixed :-D

[devth]

Looks good overall! Wana add any unit tests?

[devth]

Good idea. I think I've hardcoded Pacific Time elsewhere but obtaining from Slack user would be clever.

[jcorrado]

Tests added. This is my first time use Midje, which feels cleaner than clojure.test. I'll update this PR shortly with the refactors you suggested above (thanks!).

[devth]

Looks good! Another contributor (@dawsonfi) ported Yetibot's test from clojure.test to Midje awhile back. It seems pretty clean but I'm still on the fence to be honest 😆

Tests in yetibot.core are still using clojure.test. Maybe someday we'll unify and just pick one.

[jcorrado]

I believe this is gtg, from my end. Moving TZ and EDN bits would be a separate PR, with associated changes made elsewhere in the code base.

[devth]

@jcorrado looks good except there's a test failure!

[jcorrado]

Investigating test failure now. Unclear why that passed on my system. I wonder what I might muddied up to skew things.

[jcorrado]

I see the problem. Ginning up a fix now...

[jcorrado]

GTG from my end.

[devth]

We can replace loop with reduce but it's not exactly an improvement.

  (def words (str/split "Clojure is a general-purpose programming language with an emphasis on functional programming" #"\s"))

  (def sm (mk-slur-map (count (filter slurrable? words))))

  (:words-acc
    (reduce
      (fn [{:keys [sm words-acc]} word]
        (let [[should-slur & rest-slur-map] sm]
          (if (slurrable? word)
            ;; possibly slur it while consuming an entry from sm
            {:words-acc (conj words-acc (if should-slur (slur-word word) word))
             :sm rest-slur-map}
            ;; just accumulate the word and maintain the slur map as-is
            {:words-acc (conj words-acc word)
             :sm sm})))
      {:sm sm
       :words-acc []}
      words))

result

["Clllojurre" "is" "a" "generrraalll-purrpose" "programming" "language" "with" "aan" "emphaaasis" "on" "functionaalll" "prrogrrraamming"]

(Note I split on whitespace but of course this doesn't retain the user's original whitespace).

I'm fine with it either way; I'm not suggesting you switch to reduce as I don't think it's significantly more readable.

If I were to continue refactoring I would probably make the slur-map vector's length match the length of the words vector. It would check if a word is slurrable and put in a nil without affecting overall chance of true values for slurrables.

The nice part about making the length match is then we could map them together and simplify the slur logic quite a bit:

  (map (fn [word slur?]
         (if slur? (slur-word word) word))
       words
       slur-map)

[devth]

🎊