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

fix typespecs, replace mentat with cachex, fix flaky test, improve docs #38

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

andyleclair
Copy link
Contributor

Hi! First, thank you so much for this library! ODBC is a real pain, and it doesn't work with non-ASCII characters in a query.

I wanted to submit this to you because I know Keathley can be a bit slow, and since people have been waiting months for a proper Mentat release that matches main, I don't want to hold my breath about it. I've used Cachex, and it's a perfectly usable replacement. I've set the cache options to match Mentat, so it won't change any behavior of the janitor timing.

I also fixed a few typespecs (token was listed as only binary when it could also be keyword), updated the docs for the token auth parameter, and I fixed a flaky test caused by using Mimic across process boundaries.

Please let me know if you're interested in accepting any of these changes! Thanks again!

@forest
Copy link
Collaborator

forest commented Mar 31, 2023

@andyleclair, thanks for the contributions. I have been thinking we need to replace Mentat.

@andyleclair
Copy link
Contributor Author

I saw that linting step failed, it may be a few days until I can resolve that, my wife and I are about to go to the hospital to have a baby 😅

mix.exs Show resolved Hide resolved
@@ -37,9 +37,11 @@ defmodule Avalanche.TokenCacheTest do
-----END PRIVATE KEY-----
"""

setup :set_mimic_global
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this one? We are about to move away from Mimic. If we do, no worries and we will tackle this later

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was for the flaky tests. Let's leave it and remove when we remove Mimic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, mimic doesn't work great across process boundaries, which it would be doing here in the call to the token cache

@forest
Copy link
Collaborator

forest commented Mar 31, 2023

my wife and I are about to go to the hospital to have a baby 😅

We got you! Your wife and baby are a priority!

@pdgonzalez872
Copy link
Contributor

@andyleclair thanks for doing this!

Congrats on the baby! ❤️

@forest forest merged commit a04953d into HGInsights:main Mar 31, 2023
hgdeploy pushed a commit that referenced this pull request Mar 31, 2023
## [0.11.6](v0.11.5...v0.11.6) (2023-03-31)

### Bug Fixes

* typespecs, replace mentat with cachex, fix flaky test, improve docs (#38) ([a04953d](a04953d)), closes [#38](#38)
@hgdeploy
Copy link

🎉 This PR is included in version 0.11.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@andyleclair
Copy link
Contributor Author

Thank you both!

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

Successfully merging this pull request may close these issues.

4 participants