Skip to content
This repository has been archived by the owner on Mar 19, 2021. It is now read-only.

Get rid of warnings when compiling under 1.5.x #60 #61

Merged
merged 2 commits into from
Sep 11, 2017
Merged

Get rid of warnings when compiling under 1.5.x #60 #61

merged 2 commits into from
Sep 11, 2017

Conversation

mikestok
Copy link
Contributor

@mikestok mikestok commented Sep 6, 2017

These changes quiet the warnings emitted when compiling under Elixir
1.5.1, they also work under 1.4.4.

This addresses my issue #60
but I'm not sure if it's the "right thing" to do. That depends on how much
testing you want to do with older Elixirs.

@sourcelevel-bot
Copy link

Hello, @mikestok! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@mikestok
Copy link
Contributor Author

mikestok commented Sep 6, 2017

Excellent, CI is catching things I couldn't test locally this morning. I will fix this evening (eastern time) so that the build at least passes 😉

@mmmries
Copy link
Collaborator

mmmries commented Sep 6, 2017

Thanks @mikestok 💛 💜 ❤️ 💙 💚 💛 💜 ❤️ 💙 💚 💛 💜 ❤️ 💙 💚

I'm not sure how many people will still be using elixir 1.2.x or 1.3.x. If you can find any evidence that suggests that less than 5% of people are still on one of those versions I would be happy to drop support for them. I don't want to leave people in the cold if there is a nasty bug found somewhere in here, but if most people are keeping up with latest elixir then removing the warnings will be a definite win.

These changes quiet the warnings emitted when compiling under Elixir
1.5.1, they also work under 1.2.6, 1.3.2, and 1.4.0.
@mikestok
Copy link
Contributor Author

mikestok commented Sep 6, 2017

I think we can make it work for everyone if you're OK with the changes.

In addition to adding elixir 1.5.1 to the matrix for testing also test
with elixir 1.5.1 and otp_release 20.0.

Not sure how to make otp 20.0 work with elixir 1.2.6, 1.3.4, and
1.4.0. I believe most people on 1.5.x will use otp 20 or above.
@mmmries
Copy link
Collaborator

mmmries commented Sep 7, 2017

👍 this looks good to me. @jazzyb @scouten either of you have concerns about using the Version.compare?

@scouten
Copy link
Contributor

scouten commented Sep 7, 2017

👍 LGTM too.

I don't have any concerns about Version.compare assuming it passes CI on the targeted versions.

FWIW Ecto has recently dropped support for Elixir before 1.4.0: https://github.com/elixir-ecto/ecto/blob/master/mix.exs#L10. (Related: I've done the same in sqlite_ecto2.)

@mmmries mmmries merged commit bc5090a into elixir-sqlite:master Sep 11, 2017
@mmmries
Copy link
Collaborator

mmmries commented Sep 11, 2017

🎉 🎈 thanks @mikestok

@mmmries
Copy link
Collaborator

mmmries commented Sep 11, 2017

Released as version 1.3.3 https://hex.pm/packages/sqlitex/1.3.3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants