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

Adds support for Scalr options #37

Closed
wants to merge 2 commits into from
Closed

Adds support for Scalr options #37

wants to merge 2 commits into from

Conversation

brianium
Copy link

@brianium brianium commented May 8, 2018

fixes #29

@mikera let me know if anything jumps out as wrong/improvable :)

Due to the multi-arity signatures of resize and scale I found it difficult to create a backwards compatible change that made sense. So a couple of options seemed viable to me:

  1. Leave as is in the PR and options is now required (probably a major version change?)
  2. Convert all arguments to entries in an options or params map - i.e :new-width, :new-height (probably a major version change?) Something like:
(scale img {:height 100 :width: 100 :method :ultra-quaity})
  1. Something entirely different because I'm fairly certain I don't know everything 😄

Again, thanks a lot for this lib! Let me know what I can do to help get this PR merged 💯

@mikera
Copy link
Owner

mikera commented May 13, 2018

Thanks @brianium ! I think the extra parameters would be better specified as an additional options map argument (keyword args are a bit fiddly to compose.

Could you make this modification then I'll merge and release? Thanks!

@brianium
Copy link
Author

@mikera I think I may be a bit confused. I thought what I included in this PR was an options map (just destructured).

I just wasn't sure on how to include an optional options map and maintain backwards compatibility (if that is the goal) with the multi arity signatures of resize. So how should I include an optional options map for a function that can accept new-width and new-height and optionally options, as well as new-width and optionally options (how is this scenario distinguished from providing new-width and new-height)?

I'm finding this a bit difficult to articulate so I apologize in advance :)

The PR I submitted currently supports the following:

(resize ant 250 {:method :ultra-quality})

;; or

(resize ant 250 400 {:method :ultra-quality})

;; but how would we support the former syntaxes of no options and optionally no height?

(resize ant 250) ;;[img new-width]
(resize ant 250 400) ;; [img new-width new-height]

What if someone wants to invoke with just new-width and also pass options?

@brianium
Copy link
Author

@mikera sorry if I made a confused mess 🙍‍♂️ - let me know if there is something I can do to keep this moving along - even if its scrap it and start over 😄

@yuhan0
Copy link

yuhan0 commented Apr 3, 2019

Another way of exposing this API could be through dynamic vars?
Something like:

(def ^{:dynamic true} *scaling-options*
  {:method :balanced, :mode :fit-exact})

And at the call site:

(binding [*scaling-options* {:method :ultra-quality}]
  (resize ant 250))

@brianium brianium closed this Nov 30, 2020
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.

Access to imgscalr options
3 participants