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

test(Ash.DataLayer.Ets): add unit tests #93

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

TheFirstAvenger
Copy link
Contributor

Couldn't figure out how to create a Filter, so if you could point me in the right direction, I can add that to the PR.

Closes #51

@zachdaniel
Copy link
Contributor

@TheFirstAvenger wow! I'll take a look now :)

@coveralls
Copy link

coveralls commented Aug 17, 2020

Coverage Status

Coverage increased (+0.2%) to 61.224% when pulling ac9acb5 on TheFirstAvenger:mb-51-ets-unit-tests into 8aa94b5 on ash-project:master.

@zachdaniel
Copy link
Contributor

There is so much undocumented at the moment as I work on the elixirconf talk, so how far you got with this is very impressive, thanks so much! A filter is a keyword list, and it looks like this:

Ash.Query.filter(query, name: "zach")
# or more complex
Ash.Query.filter(query, 
  or: [
    [name: [in: ["zach", "mike"]]],
    [name: [not: [eq: "joe"]]]
  ]
)

@zachdaniel
Copy link
Contributor

If you're interested, there are a lot of improvements that could be made to the ETS data layer, like translating filters into matchspecs as opposed to fetching and filtering at runtime. I also added an is_nil predicate that I didn't support in the ETS or Mnesia data layers yet.

Let me know if you'd like to hop on a call some time :)

@TheFirstAvenger TheFirstAvenger marked this pull request as ready for review August 18, 2020 19:39
@TheFirstAvenger
Copy link
Contributor Author

@zachdaniel good to merge, 100% coverage on the Ets module. I'll take a look at the other stuff you mentioned as I have time. Looking forward to the ElixirConf talk!

@@ -135,7 +135,7 @@
{Credo.Check.Warning.ExpensiveEmptyEnumCheck, []},
{Credo.Check.Warning.IExPry, []},
{Credo.Check.Warning.IoInspect, []},
{Credo.Check.Warning.LazyLogging, []},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes to the .credo.exs required for your changes to be valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but those two are legacy checks that no longer apply and produce warnings in the logs if not disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good to know thanks!

Copy link
Contributor

@zachdaniel zachdaniel left a comment

Choose a reason for hiding this comment

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

Unless they are necessary, we should remove the .credo.exs changes from this PR.

Copy link
Contributor

@zachdaniel zachdaniel left a comment

Choose a reason for hiding this comment

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

Looks great!

🚀 Thank you for your contribution! 🚀

@zachdaniel zachdaniel merged commit d4465a2 into ash-project:master Aug 18, 2020
@TheFirstAvenger TheFirstAvenger deleted the mb-51-ets-unit-tests branch August 18, 2020 20:01
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.

Unit test the ets data layer
3 participants