image-js / image-js-typescript

Temporary repository to work on the migration of image-js to TypeScript
https://image-js.github.io/image-js-typescript/
MIT License
5 stars 5 forks source link

refactor: rename bitDepth to depth #445

Closed moonayyur closed 4 months ago

moonayyur commented 4 months ago

closes #444

stropitek commented 4 months ago

Would it be simpler to remap bitDepth to depth when passing the options to fast-png?

stropitek commented 4 months ago

Would it be simpler to remap bitDepth to depth when passing the options to fast-png?

Simpler but mainly non-breaking

stropitek commented 4 months ago

You can merge with main to get rid of the CI errors

targos commented 4 months ago

Can you please add a test that would fail before the fix? You can add it to encodePng.test.ts

moonayyur commented 4 months ago

You fix the encodePng function, so the test should execute that function, not the one from fast-png directly.

So the image must be converted to 8 bits first ? Since the encodePng function only accepts a prop of type Image where bitDepth is mandatory (so we can't test directly with the default value of fast-png)

targos commented 4 months ago

I don't see why it would have to be converted to 8 bits first. bitDepth is mandatory, but its value doesn't have to be 8.

moonayyur commented 4 months ago

Oh, okay I had misunderstood this

Can you please add a test that would fail before the fix?

I thought I should write a test that shows the previous error

targos commented 4 months ago

You should write a test that pass with the fix and doesn't pass without the fix.

moonayyur commented 4 months ago

The previous error was that the attribute depth in fast-png wasn't getting any value from img since it was named bitDepth But now bitDepth is renamed to depth inside encodePng, so I don't see how I could go back to the error in the test

targos commented 4 months ago

You're correct, and that's not what I'm asking. Somehow I'm not making myself clear but I don't know how to say it differently 😕

stropitek commented 4 months ago

The test you wrote SGTM even if you should confirm it fails by reverting the fix and re-executing the same test.