jessedoyle / prawn-icon

Easy icons for Prawn.
Other
28 stars 15 forks source link

Allow use of prawn-icon without extension #27

Closed mojavelinux closed 7 years ago

mojavelinux commented 7 years ago

For libraries like Asciidoctor PDF, it would be nice if it were possible to load the prawn/icon/font_data without loading the API extension. This would accommodate what I call low-level usage.

The key here is to move the common stuff (e.g., error types) to a shared file that is required by font_data.rb so that it's possible to only and safely require prawn/icon/font_data.

jessedoyle commented 7 years ago

We could definitely refactor the code somewhat to make this possible.

@mojavelinux - Please correct me if I'm wrong, but is your intended use case to require 'prawn/icon/font_data' without requiring prawn/icon first?

jessedoyle commented 7 years ago

After a quick initial look, FontData doesn't seem to have too many dependencies. It looks like the error references and an Icon::FONTDIR constant (here) are the main issues, and FontData seems to be the only class that references them.

As I personally find the code a little easier to follow when app-wide constants and errors are defined in a single unified location, would you be open to the creation of a Prawn::Icon::Base module that would define the FONTDIR and errors and then you could do something like:

require 'prawn/icon/base'
require 'prawn/icon/font_data'

to just get FontData and its dependencies?

mojavelinux commented 7 years ago

Exactly.

(As it turns out, Asciidoctor PDF does use the extension after all...but I still think it would be nice to be able to load the library in parts without having the side effect of loading the Prawn extension).

jessedoyle commented 7 years ago

@mojavelinux - This issue gave me a good excuse to do some housecleaning in this gem and tidy up the specs/code a little bit.

Here's a PR - let me know what you think!: https://github.com/jessedoyle/prawn-icon/pull/28

jessedoyle commented 7 years ago

Merged to master in 1e4522daa7f3afd5752f160d836add17ec219dd3.

mojavelinux commented 7 years ago

Looks great to me!

mojavelinux commented 7 years ago

Actually, I think you're still missing one of the permutations. It would be nice to be able to load the interface part without having it automatically registered as an extension. That would mean moving the interface API to prawn/icon/interface (this is similar to how prawn-svg is structured). The extension is only registered if you require 'prawn/icon'.

mojavelinux commented 7 years ago

The benefit here is that you could do a single require and have everything except the automatic registration:

require 'prawn/icon/interface'

(the base, font_data, and parser would get loaded automatically).

jessedoyle commented 7 years ago

Yea I thought about that after merging and pushing to rubygems. I'll get a PR going next week to move the interface out!

I'll reopen this issue in the meantime.

jessedoyle commented 7 years ago

@mojavelinux - See PR: https://github.com/jessedoyle/prawn-icon/pull/29. This should resolve this issue when merged!

jessedoyle commented 7 years ago

Merged #29. Version 1.3.0 released!

mojavelinux commented 7 years ago

:+1: