Skip to content

Supplying custom data readers to defconfig #18

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

Closed
lloydshark opened this issue Mar 13, 2015 · 14 comments
Closed

Supplying custom data readers to defconfig #18

lloydshark opened this issue Mar 13, 2015 · 14 comments

Comments

@lloydshark
Copy link

I'd like to optionally be able to supply custom data readers when specifying my config.

So the use of defconfig might look like the following.

(defn my-custom-reader [form]
  :some-other-value)

(defconfig my-configuration (io/resource "config.edn")
           :data-readers {'custom/my-reader my-custom-reader})

I originally thought I might have been able to achieve this by binding *data-readers*, but it turns out that the underlying edn reader doesn't allow for that (probably with good reason).

I would be happy to do the implementation for you to have a look at.

What do you think?

@jarohen
Copy link
Owner

jarohen commented Mar 13, 2015

I've solved this in Phoenix by making the edn/read-string call take *data-readers* (and, by extension, data_readers.clj) into account - more than happy to do the same here if need be. What do you think would be the best option?

Certainly happy to have a PR! Cheers

@lloydshark
Copy link
Author

Hi James, thanks for the response.

My opinion (though not strong) I think it is more flexible if you allow them to be optionally passed in.

That way you know explicitly what is legal for your config file. If you really want data-readers to be consulted you can still pass it in.

@jarohen
Copy link
Owner

jarohen commented Mar 17, 2015

Yep, good point - so would you have them passed in as an optional extra argument to defconfig, say?

@lloydshark
Copy link
Author

Yes, I was probably thinking an optional keyword argument... (just in case you ever want to add other optional parameters to defconfig) so the defconfig macro could be like the following.

(defmacro defconfig [name file-or-resource & {:keys [custom-data-readers]}]

When supplied they would be merged with the existing nomad data readers.

@jarohen
Copy link
Owner

jarohen commented Mar 18, 2015

I've just released 0.7.1-SNAPSHOT, which now takes *data-readers* into account. defconfig also takes an extra optional parameter:

(defconfig my-config (io/resource "...") {:data-readers {'my-reader/reader-type parse-reader-type}})

Let me know what you think :)

Cheers,

James

@lloydshark
Copy link
Author

Thanks James - you beat me to it... I started on the code last night. I'll try it out (hopefully today).

@lloydshark
Copy link
Author

OK, tried it out - I got the following error:

Caused by: java.lang.RuntimeException: No such var: nomad/data-readers

I think there's a missing ~ in front of data-readers in the defconfig macro.

jarohen added a commit that referenced this issue Mar 20, 2015
@jarohen
Copy link
Owner

jarohen commented Mar 20, 2015

Thanks Lloyd, sorry for that :/
Have pushed another snapshot.

James

@lloydshark
Copy link
Author

No trouble. I should have a chance today to try it out.

@lloydshark
Copy link
Author

Thanks for that. Looks good to me, worked for my simple test cases.

I tried specifying the optional extra parameter, and then without it. I didn't try binding *data-readers* directly.

@lloydshark
Copy link
Author

Hi James,

I'm wondering whether this change is likely to make it into the main release anytime soon. It certainly works for our needs.

Are you waiting for feedback on issue #19 before you would consider releasing it ?

  • Boyd

@jarohen
Copy link
Owner

jarohen commented Apr 30, 2015

Sorry - had forgotten about this - I've just a 0.7.1 release. Let me know what you think :)

James

@lloydshark
Copy link
Author

Hey no trouble. Will integrate into our main development line soon. Thanks again.

@jarohen
Copy link
Owner

jarohen commented May 6, 2015

No worries, thanks for your suggestion!

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

No branches or pull requests

2 participants