Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JESI-2210 #14

Merged
merged 6 commits into from
Sep 4, 2018
Merged

JESI-2210 #14

merged 6 commits into from
Sep 4, 2018

Conversation

axrs
axrs previously approved these changes Sep 3, 2018
axrs
axrs previously approved these changes Sep 3, 2018
@axrs axrs assigned AndreTheHunter and unassigned axrs Sep 4, 2018
Copy link
Contributor Author

@AndreTheHunter AndreTheHunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

(defn ->snake_case [s]
(some-> s
->kebab-case
(string/replace #"-" "_")))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace chars instead of creating regex:

(string/replace \- \_)

'a-snail-can-sleep-for-three-years
'slugsHaveFourNoses)
(is (= "a_snail_can_sleep_for_three_years" a-snail-can-sleep-for-three-years))
(is (= "slugs_have_four_noses" slugsHaveFourNoses))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(is (true? (bp/macro? `defconsts)))))

(testing "transforms the symbol values with the given function"
(defconsts bp/->snake_case
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does composing the body-fn work?
Since we may want a screaming snake_case, the easiest would be to: (comp string/upper-case bp/->snake_case)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. Test added.

`(do ~@(for [s symbols
:let [name (second s)
body (str name)]]
`(def ~name (~body-fn ~body)))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need to extract body-fn to a variable outside the loop, in case it's composed (see comment below)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. It's fine as is.

@AndreTheHunter AndreTheHunter assigned axrs and unassigned AndreTheHunter Sep 4, 2018
@axrs axrs assigned AndreTheHunter and unassigned axrs Sep 4, 2018
@AndreTheHunter
Copy link
Contributor Author

👍

@AndreTheHunter AndreTheHunter assigned axrs and unassigned AndreTheHunter Sep 4, 2018
@axrs axrs merged commit 4f02906 into master Sep 4, 2018
@axrs axrs deleted the JESI-2210 branch September 4, 2018 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants