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

Add options hash / Add lower casing of key names #2

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hacklschorsch
Copy link

I want to integrate your parser into the NodeJS 'node-imagemagick' module (https://npmjs.org/package/imagemagick) because the parser in that module won't digest bogon input (as I receive it from graphics magick for a couple of test images) and is not very legible.

node-imagemagick likes to have the keys of the parsed Identify output lowercased, and the IMHO best way is to add that feature to your module.

I have added a unit test.
I have not added a test for combining lowerCase=true with camelCase=true, though it is possible to do.

@dandean
Copy link
Owner

dandean commented Feb 26, 2013

Hello! Thanks for the patch. If this module is to perform different and mutually exclusive transforms, I'd prefer to not use an options object. Would you mind refactoring into something like this:

/**
 * ImageMagickIdentifyReader(text[, transform]) -> ImageMagickIdentifyReader
 * - text (String): Output text from the `identify` program.
 * - transform (String): Optional key transformation: "camel" | "lower"
 *
 * Returns a parsed object representation of the input string.
 *
 * The optional `transform` argument can instruct this module to transform keys
 * into different formats: "camel" converts keys to camelCase and "lower" to
 * lower case. For backwards compatibility, the Boolean value `true` will be
 * treated as "camel".
**/

If you're cool with this, I'll gladly merge it in.

@hacklschorsch
Copy link
Author

Hi Dan!

I don't mind at all. I like option hashes for they allow easy extending later without breaking backwards compatibility. Also, the 'lowerCase' and 'camelCase' are not exclusive, they can be combined (don't know if it makes much sense though). May I propose packing the 'transform': directive into an options hash, so the next crazy feature won't have to change the API in a breaking way again?

Never the less, I'll post the change ASAP. Glad you had the time to look at my patch so quickly!

@hacklschorsch
Copy link
Author

Also, previously, not only "the Boolean value true" (was) "treated as camel" but instead all "tru-ish" values. I'll try to respect that in my patch.

@hacklschorsch
Copy link
Author

What do you think of the patch?

Bump version. Our own deployments have to recognize whether they have the correct version of this package (until the patches are integrated upstream)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants