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

document and fragment allow conditionals and nested arrays of filter&transformer pairs #10

Closed
wants to merge 2 commits into from

Conversation

da-z
Copy link
Contributor

@da-z da-z commented Feb 24, 2013

Hi Anthony. First, let me thank you for this great library.

I just started to use it a few days ago and found that I needed more flexibility when defining my fragment and document bodies, so I made those modifications.

Maybe you find the feature useful enough to add it into the main branch.

Regards

@Raynes
Copy link
Owner

Raynes commented Feb 24, 2013

This is a great idea. I've merged your pull request, but I made some changes that I'll go over briefly:

  • ffns wasn't a very descriptive name so I renamed it to flatten-fns.
  • I don't care for the clojure.core/flatten function. It completely ignores maps! While that isn't a problem for our particular use-case, we already had an improved version we wrote for clojail so I just put that in useful and used it here instead. You didn't do anything wrong here though, it's just my OCD kicking in ;).
  • The formatting of the test you added was really bad. This was the only thing you did that I really needed to change. Clojurians almost never put brackets on their own lines like this (as you might be familiar with from languages like C, Java, and C#). It makes it nearly impossible to read for other Clojurians. It may seem better at first, but it really isn't. Always inline those brackets. You can see how I did it in the commits I added in master.

Overall, good work and thanks for the contribution. I was trying to think of the best way to do this sort of thing and I think your way is really good. I've released a new version 0.1.28 if you want to start using it right away.

@Raynes Raynes closed this Feb 24, 2013
@da-z
Copy link
Contributor Author

da-z commented Feb 25, 2013

Awesome! Grateful for all the tips too.

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