Skip to content
This repository has been archived by the owner on Apr 21, 2021. It is now read-only.

Dispatch to rules using a protocol. #172

Merged
merged 43 commits into from
Jan 31, 2016
Merged

Dispatch to rules using a protocol. #172

merged 43 commits into from
Jan 31, 2016

Conversation

lpil
Copy link
Owner

@lpil lpil commented Dec 13, 2015

See #171

Breaking changes ahoy.

  • Dogma.Rule protocol
  • defrule macro
  • Rule.Case test macro
  • All rules using defrule
  • All rules tested using Rule.Case
  • Rule sets updated to use structs
  • Dogma using protocol to run rules
  • Config generation handles new format
  • Config supports old format
  • Deprecation message for old format
  • Config documentation updated
  • No pending tests
  • Changelog

@lpil lpil force-pushed the feature/rule-protocol branch 3 times, most recently from a9e9479 to 3d50aef Compare December 13, 2015 20:54
defmodule Dogma.Rule.CommentFormat do
use Dogma.RuleBuilder

defrule Dogma.Rule.CommentFormat, [] do
Copy link
Contributor

Choose a reason for hiding this comment

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

small comment but if there are no default config values maybe

defrule Dogma.Rule.CommentFormat do

would be better.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I should do that.

@lpil lpil force-pushed the feature/rule-protocol branch 2 times, most recently from 69e80b5 to 58c6ee0 Compare January 23, 2016 14:05
@lpil
Copy link
Owner Author

lpil commented Jan 30, 2016

@meadsteve

Care to review?

Also, do you know how to go from a module atom name to the struct for that module?

i.e.

assert transform(LineLength) == %LineLength{}

I can't work it out.

@meadsteve
Copy link
Contributor

@lpil I think the kernel function struct(struct, kv \\ [])might be what you're looking for. I'll take a look at this pr today when I get a chance.

@lpil
Copy link
Owner Author

lpil commented Jan 31, 2016

Ah, thanks! I didn't think to look in kernel. :)

Dict.put( rules, rule, config )

defp insert_rule({rule_name, config}, acc)
when is_atom(rule_name) and is_list(config) do
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 like this approach to updating the config style. Neat way of emitting deprecations and the whole method can just be removed at the next major version bump.

One thing I'd think about though is whether it'd be better to do this as a logger.warn rather than just an IO.put. As it is a warning not just a message.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Currently the logger isn't running. I could add it but then I need to backport ExUnit.CaptureLog, as it isn't present in 1.0 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see. That's a shame. In any case what you've done should still be useful.


defstruct unquote(opts)
unquote(module_ast)

Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if test/2 isn't defined? Does it error on compile? (please ignore all of the following if it does error). One thought is to add a rule @behaviour so this starts something like:

quote do
   defmodule unquote(name) do
      alias Dogma.Error
      alias Dogma.Script

       @behaviour Dogma.RuleBehaviour

Copy link
Owner Author

Choose a reason for hiding this comment

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

It compiles and crashes at runtime. I'll add the behaviour so it compiles with a warning.

@meadsteve
Copy link
Contributor

A few comments from me but generally this approach looks really good. I like that the script module got smaller and more focussed.

@lpil
Copy link
Owner Author

lpil commented Jan 31, 2016

Thanks Steve!

lpil added a commit that referenced this pull request Jan 31, 2016
Dispatch to rules using a protocol.
@lpil lpil merged commit 410d880 into master Jan 31, 2016
@lpil lpil deleted the feature/rule-protocol branch January 31, 2016 21:31
@lpil lpil restored the feature/rule-protocol branch February 8, 2016 22:13
@lpil lpil deleted the feature/rule-protocol branch February 26, 2016 19:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants