Let's remove this validation here (and the new validator function).

We've made a conscious decision to avoid validating any of the user's provided rendering params across our SDK, since the only mechanism we currently have for writing and maintaining these validations would involve a massively manual and brittle engineering effort. For example: what happens if tomorrow we change the painter to accept q=max or something? Updating the validator in one library would be fairly easy, but updating the validators across every library would take a lot of time and effort, and in the meantime our users would be left out in the cold waiting to use the new painter feature.

Note that we do have plans to fix this stuff, in the future. There's some work scheduled soon to add Typescript output to imgix-url-params, which would allow us to add this level of parameter validation to all of our JavaScript libraries while keeping the maintenance burden down. I'm sure we'll learn a lot through the course of that project, and once it's done we can figure out what it would take to provide similar single-source-of-truth validators for our non-JS libraries as well.

I think your instinct to add a validator here is absolutely the correct one, but since we've consciously decided to punt on validating params in this library for the moment (but not forever), I think adding a single validator for a single parameter here will probably sow more confusion than help.

Ah, okay. You're right. I was following the lead of validating domains, but I totally get that validation could become more burdensome than it's worth. So okay, I'll remove the validation step.

@sherwinski do you see anything else here you want changed? I think I will end up sorting on those values like we discussed.

Oh and just to be clear on the details, the validator checks that the value is within the range [0, 100] for our stated output quality range. You're right that if the range changes then this validation step is toast ha.

And actually, maybe I don't understand our range for this, how should I think about output quality and it's range? As a percentage or?

I believe it's a percentage of the master image quality.

Okay, that's what I thought. I guess the next question is when would we want to deviate between 0-100%? I.e. when would the percentage need to be 120% or -20%?

The strict range for this parameter is 0-100, which is tied to the JPEG spec for variable quality, so that's not going to change. My example was more along the lines of "what if we decide to support an alternate input mode for this parameter", but the broader point is just that trying to maintain a full spec of our rendering API in our individual SDK libraries is a fool's errand.

To stay consistent with all other implementations so far, we should remove this from the class constructor and add it as an option to create_srcset. Something to the effect of:

def create_srcset(
            self, path, params={},
            start=MIN_WIDTH, stop=MAX_WIDTH, tol=TOLERANCE, disable_variable_quality=False):

I know that looks clunky, so I'm open to different naming. But you get the idea.

I also wouldn't mind seeing a validator for the disable_variable_quality flag as per imgix-rb:L180. But I will defer to your decision.

Okay yeah, I could add this validator. Just to check that the boolean flag is a boolean right?

Also, as far as the snippet above, and keeping in mind how you feel about the ruby implementation, what if we rework things a bit and just pass an options dictionary here?

I did think about this initially, just wasn't sure which way to rock the boat least.

Otherwise, if you want to add another parameter here I can, however, I do feel like the parameter list length is already long as is... not a big deal, just something to mind...

Just to check that the boolean flag is a boolean right?

Yep exactly.

however, I do feel like the parameter list length is already long as is

Agreed

what if we rework things a bit and just pass an options dictionary here?

That is one approach, and I think the quickest. But maybe it's also worth looking into using **kwargs for argument unpacking. I remember looking into that option for imgix-rb, but something about having already included params={} created usability issues. What do you think?

Yeah, **kwargs has been on my mind, how much (if any) would you want to enforce the argument signature if we explore this route?