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

replace all the rules with a map that only contains the rule itself #176

Closed

Conversation

AndreaCrotti
Copy link

With this simple mechanical change (done simply with the power of Emacs query-replace-regexp and a few manual tweaks), all the tests are still passing, and it's a starting point to extend the rules to have a more rich structure, as discussed in
#175

@AndreaCrotti
Copy link
Author

Just FYI I didn't actually touch what was in (comment forms, but that could also be ported if it makes sense.

And probably the defrules macro itself can be definitively cleaned up a bit, but let me know if you are happy with this approach first.
Thanks

@danielcompton
Copy link
Member

danielcompton commented Mar 16, 2017

Hey, I'd probably prefer to see the whole change for #175 in one PR, rather than changing all the rules partway through. This is a good start for that though.

@AndreaCrotti
Copy link
Author

Ah I see, so you mean enumerating all the rules in one go?

I just thought it would be better to get this in first because any other change would conflict since I'm basically touching every rule.
So merging this first would make it a bit easier to solve possible conflicts later on, and this was just a mechanical change so very easy to review..

And by the way should I then not touch the ryles in (comment) right?
Thanks

@danielcompton
Copy link
Member

Ah I see, so you mean enumerating all the rules in one go?

Sorry, I mean I'd rather see one patch set for #175, rather than changing the defrules macro early. I don't think there are any major refactors planned which would cause any merge conflicts with what you're working on.

And by the way should I then not touch the ryles in (comment) right?

Yep

@AndreaCrotti
Copy link
Author

AndreaCrotti commented Mar 16, 2017

Sorry I don't get how I can enumerate all the rules without actually changing the defrules macro..

The defrules macro stops working if I change the structure of the rules in any way right?

And by the way I had another thought, since we are talking about restructuring the rules, it would be probably even better to make it even more explicit.

So instead of this:

{:rule [(when-not (not ?x) . ?y) (when ?x . ?y)]...}

we could have this:

{:from (when-not (not ?x) . ?y)
 :to (when ?x . ?y)...}

Specially for some long rules this would make it even easier to understand.
What do you think?

@danielcompton
Copy link
Member

Hey, what I'm saying is, I don't want to merge a partial PR towards #175, when you provide a PR, provide all of it in one patch, i.e. the changes to the rules syntax, along with the code to allow enumerating rules.

@AndreaCrotti
Copy link
Author

Sorry for the delay but now finally I work with Clojure and use Kibit so I would definitively like to get this forward..

However there are 88 rules and I haven't really written any of them, so would like to know what is needed for this PR to be merged.
At the moment the only change is to make the definition a map with :rule so that we can more easily add :id and so on and so forth.

The defrules macro has also changed according to that.
But did you want me to add :id, and :name/description to every rule as well in the same PR?

Or the changes about enumerating should be in lein-kibit instead?
And what should be the way to configure that?
Something like a configuration file to exclude unwanted things?
http://pep8.readthedocs.io/en/release-1.7.x/intro.html#configuration

@NoahTheDuke
Copy link
Collaborator

Closing this PR in light of kibit entering maintenance status. (See this comment for further details.)

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.

3 participants