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

Fixed for 0.35.0 #29

Closed
wants to merge 1 commit into from
Closed

Fixed for 0.35.0 #29

wants to merge 1 commit into from

Conversation

bararchy
Copy link

No description provided.

@l3kn
Copy link
Collaborator

l3kn commented Jun 10, 2020

Thanks!

Do you think the version of stumpy_png should change from v4.5.2 to v5.0.0 with this?

From a quick look at the crystal docs, it seems like this would be a backwards-incompatible with crystal versions < 0.35.

@Sija
Copy link
Contributor

Sija commented Jun 10, 2020

@l3kn IIRC it should be BC

@vlazar
Copy link

vlazar commented Jun 10, 2020

@bararchy
Copy link
Author

@vlazar I think that as we are in unstable land still and pre 1.0.0 it's ok to break. I wouldn't add logic to support old crystal versions, were not there yet ;)

@oprypin
Copy link

oprypin commented Jun 10, 2020

I think it's nice to support both.

@da1nerd
Copy link

da1nerd commented Jun 11, 2020

@l3kn putting aside the fact that crystal is still pre 1.0. I'd suggest ticking with the standard, that is, go ahead and bump to 5.0 since this is a breaking change as I understand it.

@da1nerd
Copy link

da1nerd commented Jun 11, 2020

FYI there's some discussion over in this PR schovi/baked_file_system#36 (practically identical to this one) on whether or not returning slice.size.to_i64 is sufficient.

It might be good to keep an eye on the outcome of that conversation.

@bararchy
Copy link
Author

@l3kn this PR shouldn't be merged as it seems Crystal is reverting this change.

@l3kn l3kn closed this Jun 14, 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.

6 participants