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

Custom Prefix configuration #23

Merged
merged 9 commits into from Apr 8, 2017
Merged

Custom Prefix configuration #23

merged 9 commits into from Apr 8, 2017

Conversation

dawsonfi
Copy link
Contributor

@dawsonfi dawsonfi commented Apr 7, 2017

  • Extracted the prefix match to a function
  • Added configuration to allow custom prefixes

Copy link
Member

@devth devth left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just a few suggestions. Thanks for the PR!

([body prefix]
(re-find (re-pattern (str "^\\" prefix "(.+)")) body)))

(def ^:private config-prefix (:value (get-config sch/Str [:command :prefix])))
Copy link
Member

Choose a reason for hiding this comment

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

Could bake the default in here in order to avoid multi-arity has-command-prefix? function, e.g.:

(def ^:private config-prefix
  (or (:value (get-config sch/Str [:command :prefix])) "!"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loved the ideia of the default prefix logic in the config-prefix.

@@ -68,7 +79,9 @@
(when-let [parsed-cmds
(or
; if it starts with a command prefix (!) it's a command
(when-let [[_ body] (re-find #"^\!(.+)" body)]
(when-let [[_ body] (if (nil? config-prefix)
Copy link
Member

Choose a reason for hiding this comment

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

Then here you don't have to check (nil? config-prefix). Simplifies to:

(when-let [[_ body] (has-command-prefix? body config-prefix)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, with that the code is much cleaner now. Thanks for the suggestion.

@@ -52,6 +54,15 @@
; ensure prefix is actually a command
(filter #(command? (-> % second second second)))))

(defn has-command-prefix?
"Returns true if body has an command matching the prefix"
Copy link
Member

Choose a reason for hiding this comment

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

This docstring isn't quite right: this function will return the body of the command if it looks like a command, otherwise nil. The semantics match re-find. Because of this I'd probably rename the function (I typically only use ? suffix if the function actually returns a boolean). Maybe something like extract-command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i haven't noticed that the regex returned the command. I have fixed it with your suggestions.

Thanks for the help.

@dawsonfi
Copy link
Contributor Author

dawsonfi commented Apr 8, 2017

Hi @devth, i've made the requested changes. If you have any further comment, please let me know.

@devth
Copy link
Member

devth commented Apr 8, 2017

@dawsonfi thanks, looks good! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants