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

chore (file_response): implement macro for crystal version check #1918

Merged
merged 2 commits into from
Oct 13, 2024

Conversation

aarcex3
Copy link
Contributor

@aarcex3 aarcex3 commented Oct 11, 2024

Purpose

Fixes #1899

Description

Implemented a macro to check the crystal version. This is to handle the deprecation of File.readable? since version 1.13 in favour of File::Info.readable?

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

@aarcex3
Copy link
Contributor Author

aarcex3 commented Oct 11, 2024

I'm not very familiar with crystal in general.
I tried to refactor the logic and put it into src/run_macros/ but no luck.
Also, all the tests pass, but I believe I should have added one.
Any tips on how to improve it?
Thanks

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thanks for taking this on. Just a small change to user Crystal's built-in version comparison https://crystal-lang.org/api/1.14.0/Crystal/Macros.html#compare_versions(v1:StringLiteral,v2:StringLiteral):NumberLiteral-instance-method then I think we should be good.

src/lucky/file_response.cr Outdated Show resolved Hide resolved
@jwoertink jwoertink added the hacktoberfest-accepted PRs accepted for Hacktoberfest label Oct 11, 2024
@aarcex3 aarcex3 changed the title chore (file): implement macro for crystal version check chore (file_response): implement macro for crystal version check Oct 11, 2024
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for doing this. Those CI failures are unrelated and I'll fix those later. This looks good! 👍

@jwoertink jwoertink merged commit 7cc4a89 into luckyframework:main Oct 13, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PRs accepted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File.readable? is deprecated starting Crystal 1.13
2 participants